[KDE Usability] Review Request 127054: Add a Filter Toolbar Button for Dolphin

[arnav dhamija]

> On March 25, 2016, 7:43 a.m., Emmanuel Pescosta wrote:
> > dolphinmainwindow.cpp, line 548
> > <https://git.reviewboard.kde.org/r/127054/diff/11/?file=453601#file453601line548>
> >
> >     Wouldn't it be better to wire the toggleFilterBar to the toggled signal on only change the visibility state in this slot? 
> >     
> >     1. We can avoid the updateViewActions() call which updates not only the filter bar action but many more things
> >     2. The visibility state of the toggle bar always follows the action toggled state - which is easier to understand than changing the toggled state of his own trigger IMHO
> 
> arnav dhamija wrote:
>     The problem with using Toggled in the slot as I just tried it out is that it causes Dolphin to crash with a segfault. As Thomas Lubking explains it:
>         
>         The crash cause will have been the toggled signal which is prone to cause recursions when bound to slots which, well, toggle the action ;-)
>     
>     I also made a small change to dolphinviewcontainer.cpp, that is I got DolphinViewContainer::setFilterBarVisible(bool visible) to emit the signal showFilterBarChanged. Then, I removed the updateViewActions() function from toggleFilterBar and setFilterBarFocus in the dolphinmainwindow.cpp file. Also, I guess we can re-introduce an updateFilterBar function to slim down usage of updateViewAction()?
> 
> Emmanuel Pescosta wrote:
>     > causes Dolphin to crash with a segfault
>     
>     This is because the slot calls updateViewActions() which updates the action -> endless recursion which blows up the stack

I have fixed that, but now something seems to work weirdly with replacing showFilterBar's QAction::Triggered with QAction::Toggled. That is, now the eventfilter is being called by the shortcut, but weirdly enough, the shortcut is activating toggleFilterBar() and not setFilterBarFocus() as it should be. The pastebin for this can be found here: [http://pastebin.com/j5cLjbQx](http://pastebin.com/j5cLjbQx)


- arnav


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127054/#review93958
-----------------------------------------------------------


On March 19, 2016, 4:04 a.m., arnav dhamija wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127054/
> -----------------------------------------------------------
> 
> (Updated March 19, 2016, 4:04 a.m.)
> 
> 
> Review request for Dolphin, KDE Usability and Emmanuel Pescosta.
> 
> 
> Repository: dolphin
> 
> 
> Description
> -------
> 
> This patch was created to implement the feature suggested here: [https://todo.kde.org/?controller=task&action=show&task_id=966](https://todo.kde.org/?controller=task&action=show&task_id=966)
> 
> This patch adds a Filter Button to the toolbar of Dolphin. This button can be toggled to show/hide the Filter Bar.
> 
> In this patch, one might notice that there is now show_filter_bar and show_filter_bar_button in dolphinui.rc file. This was done because I found that using the same action for the menu entry and the toolbar entry would cause something to break (eg, shortcuts would stop working). 
> 
> A screenshot of the toolbar is also attached.
> 
> 
> Diffs
> -----
> 
>   dolphinmainwindow.h 7003e94 
>   dolphinmainwindow.cpp f7a7613 
>   dolphinviewcontainer.h 62f9110 
>   dolphinviewcontainer.cpp 8fea3ba 
> 
> Diff: https://git.reviewboard.kde.org/r/127054/diff/
> 
> 
> Testing
> -------
> 
> manual
> 
> 
> File Attachments
> ----------------
> 
> dolphintoolbar.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/02/12/d936e5d0-557c-45c5-9a2d-f849e5d284a6__dolphintoolbar.png
> 
> 
> Thanks,
> 
> arnav dhamija
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20160325/2de0e018/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-usability mailing list
kde-usability at kde.org
https://mail.kde.org/mailman/listinfo/kde-usability