From kde-usability Fri Mar 18 16:06:09 2016 From: =?utf-8?q?Thomas_L=C3=BCbking?= Date: Fri, 18 Mar 2016 16:06:09 +0000 To: kde-usability Subject: Re: [KDE Usability] Review Request 127054: Add a Filter Toolbar Button for Dolphin Message-Id: <20160318160609.30507.89326 () mimi ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-usability&m=145831719512110 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============9204985499971858133==" --===============9204985499971858133== Content-Type: multipart/alternative; boundary="===============8719791862372133724==" --===============8719791862372133724== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit > On März 16, 2016, 7:07 nachm., Emmanuel Pescosta wrote: > > dolphinmainwindow.cpp, line 1104 > > > > > > This solution has some disadvantages, especially that it's not possible to change the filter bar shortcut anymore. > > > > A better solution would be to install an event filter [1] on the showFilterBar action. The event filter can then catch the shortcut event [2] and handle it if needed (see the KeyPressEater example in [1] how this can be done). > > > > [1] https://doc.qt.io/qt-5/qobject.html#installEventFilter > > [2] https://doc.qt.io/qt-5/qshortcutevent.html > > arnav dhamija wrote: > Cool solution! But I'm having trouble with implementing DolphinMainWindow::eventFilter(). The action which I am targetting is showFilterBar, but that is only available in the DolphinMainWindow::setupActions() function. Therefore, I cannot pass the QAction to the eventFilter() function. > > For example: > > bool DolphinMainWindow::eventFilter(QObject *obj, QEvent *event) > { > if (obj == ???) { //How should I pass showFilterBar? Should obj==QAction? Shuld I create a new class for the filter barand then check the object? > if (event->type() == QEvent::Shortcut) {1 > ... > //use the toggleFocus function and check for the precise shortcut > return true; > } else { > return false; > } > } > > Emmanuel Pescosta wrote: > > Shuld I create a new class for the filter barand then check the object? > > Yep, please create a new class for the event filter. We can then use the same event filter for the find action ;) > > Thomas Lübking wrote: > I didn't actually try, but the shortcut event should be on every widget, ie. you won't require a filter but can just override ::event(QEvent *e) function of any widget in the window. > > You can make showFilterBar a member pointer or look up the action in the dict (but that's rather overheadish...) > > arnav dhamija wrote: > That should take care of it, but I cannot figure out how to get around the fact that > > bool DolphinMainWindow::eventFilter(QObject *obj, QEvent *event) > > ...would need a QObject while I'm testing for a showFilterBar which is a QAction. How can I get around this problem? > > arnav dhamija wrote: > Overloading the eventFilter function with different function arguements doesn't seem to work either and causes a lot of compilation problems. > > arnav dhamija wrote: > I have figured out how to setup the eventFilter now and hence my previous question is no longer valid :) But, I see the problem is that now the QAction is no longer activatable by a shortcut when I use showFilterBar->installEventFilter(this) although my eventFilter works fine otherwise. Something seems to break shortcuts from working when I do this. > > Emmanuel Pescosta wrote: > We can help you if you upload your latest patch ;) The shortcut event *should* be delivered to every object (or at least widget) in that window - that's why you likely can omit the filter and just re-implement the ::event() function of the window. - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127054/#review93616 ----------------------------------------------------------- On März 16, 2016, 4:53 nachm., arnav dhamija wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127054/ > ----------------------------------------------------------- > > (Updated März 16, 2016, 4:53 nachm.) > > > 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 > patch.txt > https://git.reviewboard.kde.org/media/uploaded/files/2016/03/16/1eb43168-65dd-410b-a74b-f895f3035c83__patch.txt > > > Thanks, > > arnav dhamija > > --===============8719791862372133724== MIME-Version: 1.0 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127054/

On März 16th, 2016, 7:07 nachm. UTC, Emmanuel Pescosta wrote:

dolphinmainwindow.cpp (Diff revision 9)
1095 
    connect(showFilterBar, &QAction::triggered, this, &DolphinMainWindow::showFilterBar);
 1098 
    QShortcut *filterBarShortcut = new QShortcut(QKeySequence( Qt::CTRL + Qt::Key_I), this);
 
This solution has some disadvantages, especially that it's not possible to change the filter bar shortcut anymore.

A better solution would be to install an event filter [1] on the showFilterBar action. The event filter can then catch the shortcut event [2] and handle it if needed (see the KeyPressEater example in [1] how this can be done).

[1] https://doc.qt.io/qt-5/qobject.html#installEventFilter
[2] https://doc.qt.io/qt-5/qshortcutevent.html

On März 17th, 2016, 11:34 vorm. UTC, arnav dhamija wrote:

Cool solution! But I'm having trouble with implementing DolphinMainWindow::eventFilter(). The action which I am targetting is showFilterBar, but that is only available in the DolphinMainWindow::setupActions() function. Therefore, I cannot pass the QAction to the eventFilter() function.


For example:


bool DolphinMainWindow::eventFilter(QObject obj, QEvent event)
{
    if (obj == ???) { //How should I pass showFilterBar? Should obj==QAction? Shuld I create a new class for the filter barand then check the object?
        if (event->type() == QEvent::Shortcut) {1
        ... 
        //use the toggleFocus function and check for the precise shortcut
           return true;
        } else {
           return false;
      }
}

On März 17th, 2016, 5:26 nachm. UTC, Emmanuel Pescosta wrote:

> Shuld I create a new class for the filter barand then check the object?

Yep, please create a new class for the event filter. We can then use the same event filter for the find action ;)

On März 17th, 2016, 5:34 nachm. UTC, Thomas Lübking wrote:

I didn't actually try, but the shortcut event should be on every widget, ie. you won't require a filter but can just override ::event(QEvent *e) function of any widget in the window.

You can make showFilterBar a member pointer or look up the action in the dict (but that's rather overheadish...)

On März 18th, 2016, 2:11 nachm. UTC, arnav dhamija wrote:

That should take care of it, but I cannot figure out how to get around the fact that 


bool DolphinMainWindow::eventFilter(QObject obj, QEvent event)


...would need a QObject while I'm testing for a showFilterBar which is a QAction. How can I get around this problem?

On März 18th, 2016, 2:13 nachm. UTC, arnav dhamija wrote:

Overloading the eventFilter function with different function arguements doesn't seem to work either and causes a lot of compilation problems.

On März 18th, 2016, 2:47 nachm. UTC, arnav dhamija wrote:

I have figured out how to setup the eventFilter now and hence my previous question is no longer valid :) But, I see the problem is that now the QAction is no longer activatable by a shortcut when I use showFilterBar->installEventFilter(this) although my eventFilter works fine otherwise. Something seems to break shortcuts from working when I do this.

On März 18th, 2016, 2:54 nachm. UTC, Emmanuel Pescosta wrote:

We can help you if you upload your latest patch ;)
The shortcut event should be delivered to every object (or at least widget) in that window - that's why you likely can omit the filter and just re-implement the ::event() function of the window.

- Thomas


On März 16th, 2016, 4:53 nachm. UTC, arnav dhamija wrote:

Review request for Dolphin, KDE Usability and Emmanuel Pescosta.
By arnav dhamija.

Updated März 16, 2016, 4:53 nachm.

Repository: dolphin

Description 

This patch was created to implement the feature suggested here: 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.

Testing 

manual

Diffs

  • dolphinmainwindow.h (7003e94)
  • dolphinmainwindow.cpp (f7a7613)
  • dolphinviewcontainer.h (62f9110)
  • dolphinviewcontainer.cpp (8fea3ba)

View Diff

File Attachments

  • dolphintoolbar.png
  • patch.txt
    		•
    --===============8719791862372133724==-- --===============9204985499971858133== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18Ka2RlLXVzYWJp bGl0eSBtYWlsaW5nIGxpc3QKa2RlLXVzYWJpbGl0eUBrZGUub3JnCmh0dHBzOi8vbWFpbC5rZGUu b3JnL21haWxtYW4vbGlzdGluZm8va2RlLXVzYWJpbGl0eQo= --===============9204985499971858133==--