First of all, nice bug you caught here…

Nevertheless I think your patch is only the easy way out, while the problem should be solved properly:

This prevents the keypress from being passed to underlying QListView, therefore preventing F from jumping to images starting with "F".

As far as I can see, F (and others?) should search in Browse mode, and activate the action in View mode. That's more complicated than removing some code, but in general Qt should allow for implementing this (either manually enable/disable the shortcut, or set a context).

The other zoom modes do not have default shortcuts.

My long-term strategy would be to add sensible shortcuts for most of the tools and actions. F is great, and for 100% normally 1 is used, however here it is already taken for rating. Sounds like a larger effort with potential for bikeshedding, but eventually this should be tackled…

In D11633#232723, @rkflx wrote:

First of all, nice bug you caught here…

Nevertheless I think your patch is only the easy way out, while the problem should be solved properly:

This prevents the keypress from being passed to underlying QListView, therefore preventing F from jumping to images starting with "F".

As far as I can see, F (and others?) should search in Browse mode, and activate the action in View mode. That's more complicated than removing some code, but in general Qt should allow for implementing this (either manually enable/disable the shortcut, or set a context).

+1

In D11633#232723, @rkflx wrote:

My long-term strategy would be to add sensible shortcuts for most of the tools and actions. F is great, and for 100% normally 1 is used, however here it is already taken for rating. Sounds like a larger effort with potential for bikeshedding, but eventually this should be tackled…

Maybe 0 for Zoom to 100%?

In D11633#232998, @ngraham wrote:

Maybe 0 for Zoom to 100%?

Go to View mode and try what happens ;)

In D11633#233006, @rkflx wrote:

In D11633#232998, @ngraham wrote:

Maybe 0 for Zoom to 100%?

Go to View mode and try what happens ;)

Oops, how embarrassing.

In D11633#232723, @rkflx wrote:

First of all, nice bug you caught here…

Nevertheless I think your patch is only the easy way out, while the problem should be solved properly:

This prevents the keypress from being passed to underlying QListView, therefore preventing F from jumping to images starting with "F".

As far as I can see, F (and others?) should search in Browse mode, and activate the action in View mode. That's more complicated than removing some code, but in general Qt should allow for implementing this (either manually enable/disable the shortcut, or set a context).

Agree, but I wasn't sure if having keyboard shortcuts only work in some contexts was acceptable behaviour. Definitely a more complicated fix.
I was thinking perhaps this could be a temporary fix while working on the above, but I suppose one can just unbind the default.

The other zoom modes do not have default shortcuts.

My long-term strategy would be to add sensible shortcuts for most of the tools and actions. F is great, and for 100% normally 1 is used, however here it is already taken for rating. Sounds like a larger effort with potential for bikeshedding, but eventually this should be tackled…

= is generally the key used in browsers for 100%.

In D11633#232723, @rkflx wrote:

As far as I can see, F (and others?) should search in Browse mode, and activate the action in View mode. That's more complicated than removing some code, but in general Qt should allow for implementing this (either manually enable/disable the shortcut, or set a context).

Agree, but I wasn't sure if having keyboard shortcuts only work in some contexts was acceptable behaviour. Definitely a more complicated fix.
I was thinking perhaps this could be a temporary fix while working on the above, but I suppose one can just unbind the default.

Looked at this again, turns out most of the work had been done, it just wasn't triggered when switching views.

+1, works for me!

When starting in Browse, View actions such as Zoom to Fit are correctly ignored

Hm, I cannot confirm that part: Even starting in Browse, I'm unable to jump to f.png when pressing f (it does work with d.png / d). Interestingly, F works for f.png.

Anyway, as mentioned in the duplicate before, the patch works. Looking at the code now, I noticed something when adding qDebug() to every call site of updateActions(), though:

# Press Enter to switch to View mode
setView();
slotViewModeChanged();
slotViewModeChanged();
slotAdapterChanged();

# Press Enter again to switch back to Browse mode
slotViewModeChanged();

I'm aware in ImageOpsContextManagerItem Gwenview does something similar as your patch is proposing, but I wondered if we can avoid those duplicate calls and at the same time make the patch simpler. Adding a call to slotViewModeChanged in ViewMainPage::reset also fixed the bug for me (but maybe call it DocumentViewController::reset then?).

Could you check whether this makes any sense?

In D11633#258938, @rkflx wrote:

When starting in Browse, View actions such as Zoom to Fit are correctly ignored

Hm, I cannot confirm that part: Even starting in Browse, I'm unable to jump to f.png when pressing f (it does work with d.png / d). Interestingly, F works for f.png.

Need to start Gwenview in Browse, i.e. by using gwenview /path/to/images.

Anyway, as mentioned in the duplicate before, the patch works. Looking at the code now, I noticed something when adding qDebug() to every call site of updateActions(), though:

# Press Enter to switch to View mode
setView();
slotViewModeChanged();
slotViewModeChanged();
slotAdapterChanged();

# Press Enter again to switch back to Browse mode
slotViewModeChanged();

I'm aware in ImageOpsContextManagerItem Gwenview does something similar as your patch is proposing, but I wondered if we can avoid those duplicate calls and at the same time make the patch simpler. Adding a call to slotViewModeChanged in ViewMainPage::reset also fixed the bug for me (but maybe call it DocumentViewController::reset then?).

Could you check whether this makes any sense?

Yep that does make more sense, and makes it much simpler. Thanks! It's essentially your patch now :)

In D11633#259014, @rkflx wrote:

I did exactly that. Perhaps your file was named F.png, while mine read f.png?

That's weird, I tried both F and f (and my files start with the lower case f), although only the latter is relevant since that is what the default Zoom to Fit shortcut is.

In D11633#259014, @rkflx wrote:

Thanks, LGTM. I'd say this can go to the stable branch (18.04.1, if you are fast with committing…).

You could still cherry-pick to the stable branch, if you want the fix to be part of 18.04.2…

In D11633#259516, @rkflx wrote:

In D11633#259014, @rkflx wrote:

Thanks, LGTM. I'd say this can go to the stable branch (18.04.1, if you are fast with committing…).

You could still cherry-pick to the stable branch, if you want the fix to be part of 18.04.2…

Why not! Done, hopefully correctly...