- Open a folder in Gwenview
- Click on image in
- Navigate over images through forward/back mouse buttons (they are buttons on left side of the mouse)
Details
- Reviewers
ngraham - Group Reviewers
Gwenview - Commits
- R260:bc53ce23e141: [MainWindow] Navigate through mouse forward/back buttons
Diff Detail
- Repository
- R260 Gwenview
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Thanks for the patch.
TEST PLAN
Tested
Please specify what your reviewer has to do to verify your patch is working correctly.
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. |
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. :) |
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. |
app/mainwindow.h | ||
---|---|---|
84 | I'm not a big fan of PHP, so arc and other PHP scripts are not welcome to me. |
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. |
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.
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)?
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.)
What fullscreen issue? Browse mode should rewritten mouse event handling of thumbnalview that I don't want to bother.
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. :)
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 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
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.
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
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
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:
- Open Gwenview in Browse Mode
- Left-click an image -> Switch to View Mode
- Press Return to go back to Browse Mode
- 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.
- I still can reproduce this:
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.
Could you add that change to this patch too so that the issues folks have mentioned get resolved and we can finally land this?