Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have recently answered a question on Stack Overflow where OP wanted to programmatically determine if TreeView's node is checked or not, among other things. My answer got accepted since everything seems to work fine.

Still, after reading MSDN documentation for TVN_KEYDOWN, I found the part in the "Return value" section that mentioned incremental search. Therefore I have decided to ask what is a proper return value for my usage of that notification and have concluded that I should return a nonzero value.

Question:

My concern is if my code is the best way for testing if the tree's node is checked or not. That is why I came here to ask for review of that code, and for possible suggestions for improving it.

Here is the tree's creation in WM_CREATE handler:

case WM_CREATE:
    {
        // this is your treeview

        TreeView = CreateWindowEx(0, WC_TREEVIEW, 
            TEXT("Tree View"), WS_VISIBLE | WS_CHILD, 
            0, 0, 200, 500, 
            hwnd, (HMENU)ID_TREE_VIEW, GetModuleHandle(NULL), NULL);

        /************ enable checkboxes **************/

        DWORD dwStyle = GetWindowLong( TreeView , GWL_STYLE);
        dwStyle |= TVS_CHECKBOXES;
        SetWindowLongPtr( TreeView , GWL_STYLE, dwStyle );

       /************ add items and subitems **********/

        // add root item

        TVINSERTSTRUCT tvis = {0};

        tvis.item.mask = TVIF_TEXT;
        tvis.item.pszText = L"This is root item";
        tvis.hInsertAfter = TVI_LAST;
        tvis.hParent = TVI_ROOT;

        HTREEITEM hRootItem = reinterpret_cast<HTREEITEM>( SendMessage( TreeView , 
            TVM_INSERTITEM, 0, reinterpret_cast<LPARAM>( &tvis ) ) );

        // add firts subitem for the hTreeItem

        memset( &tvis, 0, sizeof(TVINSERTSTRUCT) );

        tvis.item.mask = TVIF_TEXT;
        tvis.item.pszText = L"This is first subitem";
        tvis.hInsertAfter = TVI_LAST;
        tvis.hParent = hRootItem;

        HTREEITEM hTreeSubItem1 = reinterpret_cast<HTREEITEM>( SendMessage( TreeView , 
            TVM_INSERTITEM, 0, reinterpret_cast<LPARAM>( &tvis ) ) );

        // now we insert second subitem for hRootItem

        memset( &tvis, 0, sizeof(TVINSERTSTRUCT) );

        tvis.item.mask = TVIF_TEXT | TVIF_STATE; // added extra flag
        tvis.item.pszText = L"This is second subitem";
        tvis.hInsertAfter = TVI_LAST;
        tvis.hParent = hRootItem;

        // for demonstration purposes let us check this node;
        // to do that add the following code, and add the extra flag for 
        // mask member like above
        tvis.item.stateMask = TVIS_STATEIMAGEMASK;
        tvis.item.state = 2 << 12;

        HTREEITEM hTreeSubItem2 = reinterpret_cast<HTREEITEM>( SendMessage( TreeView , 
            TVM_INSERTITEM, 0, reinterpret_cast<LPARAM>( &tvis ) ) );

        // let us expand the root node so we can see if checked state is really set
        TreeView_Expand( TreeView, hRootItem, TVE_EXPAND );
    }
    return 0L;

Here is the WM_NOTIFY handler that determines if tree is checked or not:

case WM_NOTIFY:
    {
        LPNMHDR lpnmh = (LPNMHDR) lParam;

        if( lpnmh->idFrom == ID_TREE_VIEW  )  // if this is our treeview control
        {
            switch( lpnmh->code )  // let us filter notifications
            {
            case TVN_KEYDOWN:  // tree has keyboard focus and user pressed a key
                {

                    LPNMTVKEYDOWN ptvkd = (LPNMTVKEYDOWN)lParam; 

                    if( ptvkd->wVKey == VK_SPACE )  // if user pressed spacebar
                    {

                        // get the currently selected item
                        HTREEITEM ht = TreeView_GetSelection( ptvkd->hdr.hwndFrom );

                        // Prepare to test items state

                        TVITEM tvItem;

                        tvItem.mask = TVIF_HANDLE | TVIF_STATE;
                        tvItem.hItem = (HTREEITEM)ht;
                        tvItem.stateMask = TVIS_STATEIMAGEMASK;

                        // Request the information.
                        TreeView_GetItem( ptvkd->hdr.hwndFrom, &tvItem );

                        // Return zero if it's not checked, or nonzero otherwise.
                        if( (BOOL)(tvItem.state >> 12) - 1 )
                            MessageBox( hwnd, L"Not checked!", L"", MB_OK );
                        else
                            MessageBox( hwnd, L"Checked!", L"", MB_OK );

                    }
                }
                return 0L;  // see the documentation for TVN_KEYDOWN

            case NM_CLICK:  // user clicked on a tree
                {
                    TVHITTESTINFO ht = {0};

                    DWORD dwpos = GetMessagePos();

                    // include <windowsx.h> and <windows.h> header files
                    ht.pt.x = GET_X_LPARAM(dwpos);
                    ht.pt.y = GET_Y_LPARAM(dwpos);
                    MapWindowPoints( HWND_DESKTOP, lpnmh->hwndFrom, &ht.pt, 1 );

                    TreeView_HitTest(lpnmh->hwndFrom, &ht);

                    if(TVHT_ONITEMSTATEICON & ht.flags)
                    {
                        // Prepare to receive the desired information.

                        TVITEM tvItem;

                        tvItem.mask = TVIF_HANDLE | TVIF_STATE;
                        tvItem.hItem = (HTREEITEM)ht.hItem;
                        tvItem.stateMask = TVIS_STATEIMAGEMASK;

                        // Request the information.
                        TreeView_GetItem( lpnmh->hwndFrom, &tvItem );

                        // Return zero if it's not checked, or nonzero otherwise.
                        if( (BOOL)(tvItem.state >> 12) - 1 )
                            MessageBox( hwnd, L"Not checked!", L"", MB_OK );
                        else
                            MessageBox( hwnd, L"Checked!", L"", MB_OK );

                    }
                }
            default:
                break;
        }
    }
}
break;
share|improve this question
2  
Could you post the code here instead of linking to it on another Stack Exchange site? It makes the reviewing process easier for the reviewers, so they don't have to jump back and forth between sites. – syb0rg Mar 19 '14 at 21:09
1  
you can still post that here as a question for review.will remove my downvote when you edit in your code – Malachi Mar 19 '14 at 21:13
    
@syb0rg: Done. Thank you for your suggestions. I apologize, but this is my first post. Please reconsider removing the downvote. Best regards. – AlwaysLearningNewStuff Mar 19 '14 at 21:19
1  
@Malachi: Done. Thank you for your suggestions. I apologize, but this is my first post. Please reconsider removing the downvote. Best regards. – AlwaysLearningNewStuff Mar 19 '14 at 21:20
    
@AlwaysLearningNewStuff I didn't downvote in the first place. I thought it was too extreme for your case ;) – syb0rg Mar 19 '14 at 21:25

1 Answer 1

up vote 4 down vote accepted
+50

I have learned more about tree view check-boxes than I wanted to ;)

From a first glance I only have 2 observations:

  • You have 10 lines of copy pasted code in there, you should have a function for that
  • This looks terrible:

    // Return zero if it's not checked, or nonzero otherwise.
    if( (BOOL)(tvItem.state >> 12) - 1 )
    
    • Why 12 -> Magic constant ?

But then, from reading this and that, it seems that is the right way of doing it. For the sanity of whoever would maintain this, you need a lengthy comment block there because it just looks wrong. The comment should mention why you need to shift 12 positions, and that the index can be 1 or 2 ( so that is why you subtract 1 ).

I find it funny that there is a #define INDEXTOSTATEIMAGEMASK(i) ((i) << 12) provided but no #define STATEIMAGEMASKTOINDEX(i) ((i) >> 12), perhaps you could create this macro yourself and make the code more readable by employing that macro.

As an aside, there is a 'cleaner' way of getting the state, but that works only as of Vista, so I would stick to your approach.

share|improve this answer
    
Thank you for your detailed review. Upvoted. I will wait 3 more days to see if anything else pops up before accepting your answer officially. Best regards. – AlwaysLearningNewStuff Mar 24 '14 at 20:37
1  
I doubt my code can be improved beyond your suggestions, so I am officially accepting your answer. I have already upvoted, and I have awarded the bounty points as well. Thank you for your time and useful suggestions. Best regards. – AlwaysLearningNewStuff Mar 26 '14 at 9:44

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.