Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add focus sidebar action to window #2353

Merged
merged 9 commits into from
Oct 8, 2024
Merged

Add focus sidebar action to window #2353

merged 9 commits into from
Oct 8, 2024

Conversation

jeremypw
Copy link
Collaborator

@jeremypw jeremypw commented Oct 31, 2023

Possible solution to allow users access to the sidebar without resorting to the mouse by pressing <Ctrl>Left. Note that the sidebar can already be unfocused with the shortcut <Ctrl>Right.

Partially addresses #51.

A possible alternative, or additional, solution would be to have a "Rename currently focused bookmark" action with an accelerator such as <Ctrl>F2.

@jeremypw jeremypw requested a review from a team July 14, 2024 18:14
@jeremypw jeremypw marked this pull request as ready for review July 14, 2024 18:15
@zeebok
Copy link
Contributor

zeebok commented Aug 18, 2024

While the code seems good, when I test on an OS8 vm it does not seem to move focus to the sidebar.

@jeremypw
Copy link
Collaborator Author

jeremypw commented Aug 18, 2024

@zeebok Thanks for looking at this! I'll look at it again now I am working on OS8 Wayland.

@jeremypw jeremypw marked this pull request as draft August 18, 2024 17:35
@jeremypw
Copy link
Collaborator Author

@zeebok Just tested under Wayland and X on OS8 and it works for me. It did stop working after a system update but reinstalling the branch fixed that. Maybe something to do with the vm or maybe that key combination is used for something else on your system?

@jeremypw jeremypw marked this pull request as ready for review September 8, 2024 18:32
@jeremypw
Copy link
Collaborator Author

jeremypw commented Sep 8, 2024

It is an open question whether we should also facilitate focusing the Storage listbox. After this PR is merged (which only focuses the Bookmark list) the Storage list can be focused by pressing Tab (<Shift>Tab to go the other way) but this is not so obvious.

@danirabbit
Copy link
Member

danirabbit commented Sep 10, 2024

I don't think this solves #51. I feel like OP is right that the current focus behavior is just weird. Gonna comment on the original issue

Hm actually I'm not sure how to solve this really. It feels weird in general but I don't know what the solution is right now.

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't totally love this, but it works. I tried something else with move_focus but it seems like it always puts focus inside the device list. Just one requested change because of terminal warning

src/View/Sidebar/SidebarWindow.vala Show resolved Hide resolved
@jeremypw
Copy link
Collaborator Author

jeremypw commented Sep 11, 2024

@danirabbit I agree it is hard to find a good solution for the case where a user is using both mouse/touch and keyboard.

Maybe the focus should not switch to the view when a bookmark is activated with the keyboard? But that still does not fix #51. I do not that think that issue is really fixable since we do not know when a user clicks on a bookmark what it is that they intend to do next.

Either way, I imagine a keyboard (only) user would appreciate a convenient way of focusing the bookmarks that works whatever else is focused. Not sure what to do about focusing the storage or network entries though.

@zeebok
Copy link
Contributor

zeebok commented Sep 17, 2024

Hm actually I'm not sure how to solve this really. It feels weird in general but I don't know what the solution is right now.

Since clicking on a bookmark causes no item to be selected, can we leverage that to have the rename action move to the last click item, in this case the bookmark?

@jeremypw
Copy link
Collaborator Author

jeremypw commented Sep 18, 2024

@zeebok This is a possibility, provided that as soon as a directory view item is selected then the rename action acts on that. It would still be not possible to rename the bookmark using the keyboard once the view has been used. The only solution for that (other than this one) would be to have a different shortcut for renaming bookmarks. It is a bit weird to have the same shortcut for two different actions anyway.

@zeebok
Copy link
Contributor

zeebok commented Oct 5, 2024

It would still be no possible to rename the bookmark using the keyboard once the view has been used.

I think this would probably be fine, given the context of actions that have happened. Might be worth asking the issuer what they think since it is a scenario they are in more than we are?

It is a bit weird to have the same shortcut for two different actions anyway.

Totally agree, this is a very weird problem to solve hahaha

@jeremypw
Copy link
Collaborator Author

jeremypw commented Oct 6, 2024

Is there anything about this that blocks merging (even if it is not a full or ideal solution for the renaming issue)? As we already have <Ctrl>Right to move focus from sidebar to view a matching shortcut to move focus the other way seems logical.

@zeebok
Copy link
Contributor

zeebok commented Oct 8, 2024

I don't think!

Copy link
Contributor

@zeebok zeebok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The <Ctrl>Right is a little odd, I have to hit Down after to navigate the pane and highlight and it always skips down to the second row.

@jeremypw
Copy link
Collaborator Author

jeremypw commented Oct 8, 2024

@danirabbit Are you OK for this to be merged, at least pro tem? I'll fix the mentioned <Ctrl><Right> issue separately as it is outside the scope of this PR.

@danirabbit danirabbit enabled auto-merge (squash) October 8, 2024 18:53
@danirabbit danirabbit merged commit 2f0273b into main Oct 8, 2024
4 checks passed
@danirabbit danirabbit deleted the focus-sidebar-action branch October 8, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants