[MainWindow] Navigate through mouse forward/back buttons
ClosedPublic

Authored by anthonyfieroni on Aug 3 2018, 4:04 PM.

Details

Test Plan
  1. Open a folder in Gwenview
  2. Click on image in
  3. Navigate over images through forward/back mouse buttons (they are buttons on left side of the mouse)

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
anthonyfieroni requested review of this revision.Aug 3 2018, 4:04 PM
anthonyfieroni created this revision.
rkflx added a comment.Aug 3 2018, 4:06 PM

Thanks for the patch.

TEST PLAN
Tested

Please specify what your reviewer has to do to verify your patch is working correctly.

anthonyfieroni edited the test plan for this revision. (Show Details)Aug 3 2018, 4:10 PM
rkflx requested changes to this revision.Aug 3 2018, 4:24 PM

Thanks, I'll have to look for such a device. Meanwhile, I've got a couple of inline comments.

app/mainwindow.cpp
973

Unrelated whitespace change, please revert.

1509

Unrelated whitespace change, please revert.

app/mainwindow.h
84

Current master is using override instead of Q_DECL_OVERRIDE;, meaning your patch does not apply.

Please rebase on master.

This revision now requires changes to proceed.Aug 3 2018, 4:24 PM
anthonyfieroni added inline comments.Aug 3 2018, 4:28 PM
app/mainwindow.h
84

@ngraham wants it for 18.08

Remove unrelated changes.

ngraham added inline comments.Aug 3 2018, 4:30 PM
app/mainwindow.h
84

I don't recall saying that. :) This should land on master since it's a new feature. Besides, if you intended this for 18.08, it would be nice to use arc so that the parent branch is specified automatically. :)

rkflx added inline comments.Aug 3 2018, 4:31 PM
app/mainwindow.h
84

We are past the Beta (even past the RC), so it will be in 18.12.

You are adding a new event handler to MainWindow, that's not material for stable.

anthonyfieroni added inline comments.Aug 3 2018, 4:35 PM
app/mainwindow.h
84

I'm not a big fan of PHP, so arc and other PHP scripts are not welcome to me.

Rebase to master

rkflx added inline comments.Aug 3 2018, 4:38 PM
app/mainwindow.h
84

You could at least add more context to your diff for future Diffs (there should be an argument to diff for that), so Phabricator would not show "Context not available", which makes reviewing harder.

Update to show context

rkflx requested changes to this revision.Aug 3 2018, 5:07 PM

Okay, seems nobody around here has such a mouse anymore, but I think in View mode this is working fine.

However, is this working for you in Browse mode too? I think it should work everywhere, because the keyboard shortcuts Space and Backspace as well as the corresponding buttons are working in both modes as well.

This revision now requires changes to proceed.Aug 3 2018, 5:07 PM

It works everywhere, i use such a mice for years (more than 5) you miss a lot :)

rkflx added a comment.Aug 3 2018, 5:15 PM

It works everywhere, i use such a mice for years (more than 5) you miss a lot :)

At least I don't get a MainWindow::mousePressEvent when in Browse mode. Do you know what's making it work for you (somewhere in the stack there must be something handling Qt::ForwardButton)?

Yeah, click in Browse window buttons are not handled, it's needed there ?

rkflx added a comment.Aug 3 2018, 5:30 PM

Yeah, click in Browse window buttons are not handled, it's needed there ?

Strange, in your previous comment it was still working fine. As for your question, see D14583#302943.

It's not strange, if you click at main window it works, when you click in browse window it's not, that's normal.

It has a problems with mouse click inside ThumbnailView viewport, when forward/back works, left/right not and opposite.

There are two more problem I noticed, this time in View mode:

  • Repeatedly clicking fast on one of the forward or backward buttons will only change the image for every second click.
  • Doing the same for videos will accidentally trigger fullscreen mode.

(Thanks to @muhlenpfordt, binding xdotool click 8 / 9 to a keyboard shortcut worked great in uncovering those issues.)

rkflx requested changes to this revision.Aug 6 2018, 7:45 PM

Thanks for the update. This leaves us with the Browse mode and Fullscreen issues.

This revision now requires changes to proceed.Aug 6 2018, 7:45 PM
anthonyfieroni added a comment.EditedAug 6 2018, 8:01 PM

What fullscreen issue? Browse mode should rewritten mouse event handling of thumbnalview that I don't want to bother.

rkflx added a comment.Aug 6 2018, 8:04 PM

What fullscreen issue?

See D14583#304542.

Browse mode should rewritten mouse event handling of thumbnalview that I don't want to bother.

Hm, I don't think it's reasonable to expect others to fix the issue for you, and I'm not yet convinced we should land changes which add known bugs, i.e. inconsistent behaviour depending on where you position your mouse.

It looks like in Browse view event is handled by pos and if mouse is over thumbnail it's selected otherwise selection is moved but unhighlighted

I found an old MS 5-button mouse at the office, so I can test with real hardware now. :)

There are two more problem I noticed, this time in View mode:

  • Repeatedly clicking fast on one of the forward or backward buttons will only change the image for every second click.

I don't think this is fixed. As far as I can see MainWindow::mouseDoubleClickEvent() is never called, only AbstractImageView::mouseDoubleClickEvent() is triggered.

  • Doing the same for videos will accidentally trigger fullscreen mode.

I can confirm this. Double clicking the navigation buttons while a video is active (in View Mode) triggers fullscreen.

In Browse Mode the behaviour is strange. Depending on where you click, the selection is moved but immediately cleared or Gwenview switches to View Mode.

I found an old MS 5-button mouse at the office, so I can test with real hardware now. :)

There are two more problem I noticed, this time in View mode:

  • Repeatedly clicking fast on one of the forward or backward buttons will only change the image for every second click.

I don't think this is fixed. As far as I can see MainWindow::mouseDoubleClickEvent() is never called, only AbstractImageView::mouseDoubleClickEvent() is triggered.

I see it's not called, but i can't confirm it changed on second click, it can't do more on this patch.

  • Doing the same for videos will accidentally trigger fullscreen mode.

I can confirm this. Double clicking the navigation buttons while a video is active (in View Mode) triggers fullscreen.

So that's not a problem of this patch, you can double click by wheel and it triggered too, it'll by double click on every mouse button.

In Browse Mode the behaviour is strange. Depending on where you click, the selection is moved but immediately cleared or Gwenview switches to View Mode.

I investigate and i don't see how it handled by pos but i'm pretty sure it is, case if pos is over thumb it is selected if not selection is removed, i was pretty sure the problem is mIndexUnderCursor in previewitemdelegate.cpp

rkflx resigned from this revision.Aug 25 2018, 8:35 AM
Restricted Application added a project: Gwenview. · View Herald TranscriptAug 25 2018, 8:35 AM
anthonyfieroni edited reviewers, added: muhlenpfordt; removed: rkflx.Sep 17 2018, 11:32 AM

What you expect to change on it?

What you expect to change on it?

IMO the behaviour should be consistent for View and Browse Mode. If this is not feasible maybe it's better to not introduce this feature, sorry.
The main problem I see is the Browse Mode part. It should not depend on where you press the navigation buttons whether this or another action is triggered.

Issues in Browse mode persist without the patch, you can click with Forward/Backward button over image, it's selected.

Issues in Browse mode persist without the patch, you can click with Forward/Backward button over image, it's selected.

Ok, but I think if Gwenview officially supports these buttons this should work anywhere.

I had a quick look at the Browse Mode problem and tried adding the following event handler:

void ThumbnailView::mousePressEvent(QMouseEvent* event)
{
    if (event->button() == Qt::ForwardButton || event->button() == Qt::BackButton) {
        return;
    }
    QListView::mousePressEvent(event);
}

This does not work in every situation but maybe it's a starting point for you.

  • After (left-)clicking an image to view and pressing Return to come back to Browse Mode, pressing a navigation button while over a thumbnail switches to View Mode again
  • Subfolders and archives are ignored in navigating by mouse buttons

This does not work in every situation but maybe it's a starting point for you.

It's material to another patch, see D15612

  • After (left-)clicking an image to view and pressing Return to come back to Browse Mode, pressing a navigation button while over a thumbnail switches to View Mode again

I can't reproduce, double click an image pressing return works as pressing key backward or button backward

  • Subfolders and archives are ignored in navigating by mouse buttons

Yes, it's normal, you can see actions previous and next are disabled

Will test again after you land D15612

Just tested all previous mentioned issues on current master:

  • "Repeatedly clicking fast on one of the forward or backward buttons will only change the image for every second click"
    • This still exists in View Mode (not in Browse Mode)
  • "Doing the same for videos will accidentally trigger fullscreen mode"
    • Still exists if the second of the double clicks happens on the now selected video
  • "After (left-)clicking an image to view and pressing Return to come back to Browse Mode, pressing a navigation button while over a thumbnail switches to View Mode again"
    • I still can reproduce this:
      1. Open Gwenview in Browse Mode
      2. Left-click an image -> Switch to View Mode
      3. Press Return to go back to Browse Mode
      4. If you did not move the mouse and it's still over the image which was clicked - if you now press a navigation button the prev/next image is selected and switched to View Mode.

As i mention before, that's not a patch issues.

As i mention before, that's not a patch issues.

But they become issues when this feature is added. Any chance you could work on fixing them so we can finally land this patch? :)

If someone steal events it's not problem of the patch, similar patch as D15612 should be applied for that view.

If someone steal events it's not problem of the patch, similar patch as D15612 should be applied for that view.

Could you add that change to this patch too so that the issues folks have mentioned get resolved and we can finally land this?

Test now, i can't reproduce behavior mentioned above.

ngraham accepted this revision.Fri, Feb 22, 4:39 PM

Thanks, this works great now!

This revision is now accepted and ready to land.Fri, Feb 22, 4:39 PM
This revision was automatically updated to reflect the committed changes.