Review Request 112178: Make use of the already existing DolphinNewFileMenuObserver singleton class to achieve a better error handling (DolphinNewFileMenu)

[Frank Reininghaus]

> On Aug. 27, 2013, 10:29 a.m., Frank Reininghaus wrote:
> > Thanks for the new patch, but now the "File -> Create New..." menu in Dolphin is broken.
> > 
> > It doesn't matter much if the name of the action is "create_new" or "new_menu", but if you use a single action everywhere, then the action must have the same name everywhere (including all *.rc files).
> 
> Emmanuel Pescosta wrote:
>     I haven't found any usages of "create_new" in Dolphin (except of DolphinContextMenu - 1 line), so I decided to change "create_new" in DolphinContextMenu to "new_menu".
>     
>     > then the action must have the same name everywhere (including all *.rc files)
>     Where in Dolphin have you found "create_new" usages?
> 
> Emmanuel Pescosta wrote:
>     Hmm I mean DolphinNewFileMenu not DolphinContextMenu. Sry

> Where in Dolphin have you found "create_new" usages?

In dolphinui.rc. This file defines the actions for the menu bar and the tool bar, one of which is "create_new". Similarly, the DolphinPart has the action "new_menu" in its dolphinpart.rc file.

However, these only work if a corresponding action is created in the code. Currently, this is the case, but if you merge both actions to one, which has a single name, then you have to modify one of the .rc files as well. It doesn't matter if it's "create_new" or "new_menu", the only thing that matters is that the name of the action is the same in both .rc files and in the constructor of DolphinNewFileMenu.

If the name in the constructor is different from the one in either .rc file, then the menu action in either Dolphin or the DolphinPart will not be there.

Feel free to push this patch after fixing the problem.


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112178/#review38724
-----------------------------------------------------------


On Aug. 25, 2013, 10:13 a.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112178/
> -----------------------------------------------------------
> 
> (Updated Aug. 25, 2013, 10:13 a.m.)
> 
> 
> Review request for Dolphin and David Faure.
> 
> 
> Description
> -------
> 
> Replaced all KNewFileMenu usages in DolphinPart by DolphinNewFileMenu.
> 
> Removed all signal-slot-connections related to DolphinNewFileMenu->errorMessage(QString)
> in DolphinMainWindow and DolphinContextMenu and replaced it by a better solution.
> 
> Now we make use of the already existing DolphinNewFileMenuObserver singleton class to achieve a better
> error handling, because every newly created DolphinContextMenu instance registers himself by DolphinNewFileMenuObserver
> and we use this to connect the errorMessage(QString) signal of every DolphinContextMenu instance to the errorMessage(QString)
> signal of the DolphinNewFileMenuObserver singleton class.
> 
> So we need only one connection from DolphinNewFileMenuObserver to DolphinMainWindow (or to DolphinPart) to
> collect all error messages thrown by every DolphinNewFileMenu instance.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/CMakeLists.txt 6856991 
>   dolphin/src/dolphincontextmenu.h c9840ea 
>   dolphin/src/dolphincontextmenu.cpp 3deeb38 
>   dolphin/src/dolphinmainwindow.cpp 5cc608f 
>   dolphin/src/dolphinnewfilemenu.h ae58813 
>   dolphin/src/dolphinnewfilemenu.cpp 480889f 
>   dolphin/src/dolphinpart.h 172bfaf 
>   dolphin/src/dolphinpart.cpp de4c607 
>   dolphin/src/views/dolphinnewfilemenuobserver.h 726122c 
>   dolphin/src/views/dolphinnewfilemenuobserver.cpp 1cb5739 
> 
> Diff: http://git.reviewboard.kde.org/r/112178/diff/
> 
> 
> Testing
> -------
> 
> All error messages are shown properly.
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20130827/987b00c5/attachment.htm>