Migrate from GitHub to SourceForge with this tool. Check out all of SourceForge's recent improvements.
Close

#2711 Shell Context Menu in Folder Compare view

Trunk
closed-accepted
nobody
GUI (476)
7
2008-12-16
2008-10-23
Paul
No

The patch is for the following Feature Request http://winmerge.org/rfe/2015104

I've implemented ability to add Shell Context Menu as submenu to context menu in folder comparison view. 2 new submenus appear at the bottom of the context menu: "Shell context menu for Left" and "Shell context menu for Right". If it is impossible to show context menu for currently selected group of files, appropriate submenu is not added.

There is also a checkbox in Options dialog that can enable or disable this feature (you may want to disable it because first request of shell context menu takes some time because WinMerge loads all shell extension DLLs). It is disabled by default.

The patch is against trunk HEAD (rev.6030 for the moment of submitting).

Discussion

1 2 > >> (Page 1 of 2)
  • Paul

    Paul - 2008-10-23
     
  • Kimmo Varis

    Kimmo Varis - 2008-10-23

    Thanks for the patch!

    I think the general idea is good indeed. The downside is the shell menu also contains submenus so the selections may come a bit tricky. But then again this patch probably solves few other RFE items too in a clean way.

    Some quick comments about the patch itself.

    ** Src/DirView.cpp:

    > +#define APSTUDIO_INVOKED // to see _APS_NEXT_COMMAND_VALUE
    > #include "resource.h"
    > +#undef APSTUDIO_INVOKED
    Nice trick, but it doesn't work in practice. For various reasons most of the time we don't update resource.h with Visual Studio so it is common that limit values are out of synch. Better way is to simply add a new define value.

    > +const UINT leftCmdFirst = _APS_NEXT_COMMAND_VALUE;
    > +const UINT leftCmdLast = leftCmdFirst + 0x1fff;
    > +const UINT rightCmdFirst = leftCmdLast + 1;
    > +const UINT rightCmdLast = rightCmdFirst + 0x1fff;
    Same comment than for earlier diff. And you really need to reserve so many IDs?

    ** Src/PidlContainer.cpp:
    +++ Src/PidlContainer.cpp
    This looks like generally useful class, so adding it to Src/Common makes that more clear its not WinMerge dependent. Yes, this Src and Src/Common division isn't very clear, but usually we try to add generally (not dependent on WinMerge core classes) into Src/Common.

    ** Src/Merge.rc
    > PUSHBUTTON "Reset",IDC_RESET_ALL_MESSAGE_BOXES,178,169,50,14,NOT
    > WS_TABSTOP
    > + CONTROL "Enable Shell &Context Menu",IDC_ENABLE_SHELL_CONTEXT_MENU,
    > + "Button",BS_AUTOCHECKBOX | WS_TABSTOP,7,151,220,10
    This breaks tab-order. Add items in order the tab-key moves between them.

    This patch also adds quite a bit of new code into CDirView class. Which is already too big class for my taste. Would it make sense to add new class which contains those new menu pointers and has methods you now have added to CDirView?

    This is a good start for a new feature patch. :)

     
  • Paul

    Paul - 2008-10-23

    Thanks for the comments. I'll try to answer some of your questions.

    > For various reasons most of the time we don't update resource.h with Visual Studio so it is сommon that limit values are out of synch. Better way is to simply add a new define value.

    Is there any #define already or should I add one?

    > And you really need to reserve so many IDs?

    The main idea is not to overlap with existing command IDs and do not exceed 0x7FFF. No reason to economize, but it won't work properly if there are not enough IDs.

    > adding it to Src/Common makes that more clear its not WinMerge dependent.

    Ok, I'll move it there in next patch.

    > This breaks tab-order. Add items in order the tab-key moves between them.

    Will be fixed.

    > Would it make sense to add new class which contains those new menu pointers and has methods you now have added to CDirView?

    I'll try to refactor it and move everything related to shell context menu into separate class.

     
  • Paul

    Paul - 2008-10-31

    Here is an updated patch against trunk HEAD (rev.6055).

    Shell context menu functionality is moved to a separate class — CShellContextMenu.
    Tab order was OK as far back as in previous patch, I've checked — it is correct.
    PidlContainer.* moved to Src\Common.

    Please check it, maybe there is something else that needs to be fixed.

     
  • Kimmo Varis

    Kimmo Varis - 2008-11-04
    • milestone: --> Trunk
     
  • Kimmo Varis

    Kimmo Varis - 2008-11-04

    Thanks a lot for working with this! I took a quick look and this looks great overall.

    As a minor comment, please don't add editor-specific "rule-lines" like:
    +/* vim: set noet: */
    into the files.

    I'll need some more time to look at this and do some testing, I'll post more comments later.

     
  • Kimmo Varis

    Kimmo Varis - 2008-11-19

    Been busy with other things past days. But I will arrange time for this tomorrow. I'm doing some refactoring in folder compare so this must be applied before that work. Increasing priority and assigning to me temporarily.

     
  • Kimmo Varis

    Kimmo Varis - 2008-11-19
    • priority: 5 --> 7
    • assigned_to: nobody --> kimmov
     
  • Kimmo Varis

    Kimmo Varis - 2008-11-19

    I tested this a bit myself, and I like it already. Earlier I didn't realize this for example allows me to commit changed files to Subversion from WinMerge! Which is really cool!

    There is a small delay in opening context menu for a first time when the shell context menu is enabled. But I'm afraid that that is something we must live with. And hence the default option value to disable the menu sounds good. Maybe we later find a way to make it faster...

    GUI-wise I think the menuitem text in WinMerge context menu is too long. Many translations will have much longer text. Maybe "Left Shell menu" and "Right Shell menu" would be better?

    The next step will be that I'll look through the code and unless I find something that *really* needs fixing I'll commit the patch as is now. Then the texts (and possible other things) can be tweaked in follow-up patches. I'm hoping to do a next experimental release tomorrow so this patch would be a very nice to get into it.

     
  • Paul

    Paul - 2008-11-19

    > this for example allows me to commit changed files to Subversion from WinMerge! Which is really cool!

    Actually, that's why I started implementing it in July — to be able to work with TortoiseSVN :)

    > There is a small delay in opening context menu for a first time when the shell context menu is enabled.

    That's because Shell Extensions DLLs that are used in context menu are being loaded into WinMerge. No idea how to make it faster :(

    > GUI-wise I think the menuitem text in WinMerge context menu is too long. Many translations will have much longer text. Maybe "Left Shell menu" and "Right Shell menu" would be better?

    Name them as you wish :)

     
  • Kimmo Varis

    Kimmo Varis - 2008-11-19

    Committed to SVN trunk with some trivial changes:
    - style changes (start global variables with capital letter, some others)
    - doxygen comment formatting
    - moved the option checkbox in options-dialog after other checkboxes

    Completed: At revision: 6099

    I also noticed one easily reproduceable crash:
    #1 enable the option
    #2 compare two folders
    #3 open the folder compare context menu (no need to open shell menu)
    #4 do a rescan for the folder compare (press F5)

    And there is a assert dialog in debugger...

     
  • Kimmo Varis

    Kimmo Varis - 2008-11-19
    • assigned_to: kimmov --> nobody
     
  • Kimmo Varis

    Kimmo Varis - 2008-11-19

    Changed the menuitem texts to:
    - Left Shell menu
    - right Shell menu

    Completed: At revision: 6100

     
  • Kimmo Varis

    Kimmo Varis - 2008-11-19

    VS2008 doesn't seem to know about IContextMenu*Ptr types. Might be a difference in Platform/Windows SDK headers? Anyway, I'll fix the VS2008 build with attached patch which replaces IContextMenu*Ptr with IContextMenu? *.

    File Added: ShellMenu_VS2008_buildfix.patch

     
  • Kimmo Varis

    Kimmo Varis - 2008-11-19

    Committed the build fix to SVN trunk:
    Completed: At revision: 6101

    Btw, any idea about the crash I mentioned below?

     
  • Paul

    Paul - 2008-11-20

    > VS2008 doesn't seem to know about IContextMenu*Ptr types. Might be a difference in Platform/Windows SDK headers?

    Yes, these types should be defined in Platform SDK, I forgot to rebuild with native headers before submitting path :(

    > Anyway, I'll fix the VS2008 build with attached patch which replaces IContextMenu*Ptr with
    IContextMenu? *.

    It will work, but will leak. I'll submit a fix today.

     
  • Paul

    Paul - 2008-11-20

    > Btw, any idea about the crash I mentioned below?

    I'll check it ASAP.

     
  • Paul

    Paul - 2008-11-20

    I've added 2 patches:
    ShellMenu_IContextMenuPtr_fix.rev6104.patch adds missing declarations for IContextMenu*Ptr types.
    ShellMenu_Assert_fix.rev6104.patch fixes assertion failure.

    Both of them are against revision 6104 (HEAD for now).

     
  • Paul

    Paul - 2008-11-20

    Forgot to mention: I don't have VS2008, but I've checked with VS2005 which also did not know about IContextMenu*Ptr types. Now it compiles OK, VS2008 also should, please check it.

     
  • Kimmo Varis

    Kimmo Varis - 2008-11-20

    > > VS2008 doesn't seem to know about IContextMenu*Ptr types.
    > > Might be a difference in Platform/Windows SDK headers?

    > Yes, these types should be defined in Platform SDK, I forgot
    > to rebuild with native headers before submitting path :(

    What do you mean with native headers? VS2003 and later always come (if you install it) with platform SDK. So the difference cannot be there wouldn't be Platform SDK headers available with VS2008. Yes, I know stand-alone PSDK installer has many different "parts" one can install and probably the set installed with VS doesn't contain all of them.

    Anyway, PSDK is documented to be required for WinMerge compiling. As we need things like HTML help headers from there. So perhaps it just needs new part of PSDK to be installed? Which we should just document then.

    I verified that build works with ShellMenu_IContextMenuPtr_fix.rev6104.patch for VS2003, VS2005 and VS2008 and committed it:

    Completed: At revision: 6105

     
  • Kimmo Varis

    Kimmo Varis - 2008-11-20

    Verified that ShellMenu_Assert_fix.rev6104.patch fixes the assert after rescan and committed the patch:

    Completed: At revision: 6106

    Thanks a lot for fast fixes!

     
1 2 > >> (Page 1 of 2)

Log in to post a comment.