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

revert: "feat: scroll to top on Home (#1251)" #1259

Merged
merged 1 commit into from
Dec 12, 2024
Merged

revert: "feat: scroll to top on Home (#1251)" #1259

merged 1 commit into from
Dec 12, 2024

Conversation

GeopJr
Copy link
Owner

@GeopJr GeopJr commented Dec 12, 2024

This reverts commit 7f8c8d3.

revert: #1251
fix: #1258

Global action + AdwDialogs = bad. I quickly tried setting it as a view action or a shortcut, same results. No time to debug this rn we are too close to a release

@GeopJr GeopJr changed the title reevert: "feat: scroll to top on Home (#1251)" revert: "feat: scroll to top on Home (#1251)" Dec 12, 2024
@GeopJr GeopJr merged commit 66f71ca into main Dec 12, 2024
5 checks passed
@GeopJr GeopJr deleted the revert/stp branch December 12, 2024 02:56
@toolstack
Copy link
Contributor

toolstack commented Dec 16, 2024

@GeopJr if you let me know what the issue is I can take a look. The home routines are basically the same as the pgup/pgdn code.

Edit: ok, I looked at the bug report, will look more into it this week.

@toolstack
Copy link
Contributor

Also confirmed that pgup/dn are broken in dialogs as well.

@GeopJr
Copy link
Owner Author

GeopJr commented Dec 16, 2024

We are a bit cornered with this one. Moving the action to the view should be the appropriate solution but it's not, because the view needs to be focused for them to work but IIRC people complained about the pgup/dn buttons not working when they are focused on the sidebar for example.

Home specifically is a bit more critical because it's used in text fields to go back to the start and moving at the top can be far more annoying than an unwanted pgup/dn.

Technically this wouldn't be an issue prior to Adw.Dialogs, since the app actions wouldn't be trigger-able in modal windows but since Adw.Dialogs are displayed in the window, they are (ref: #1055)

I don't know what the best approach would be right now 🤷

@toolstack
Copy link
Contributor

My initial thought would be to disable the nav accels (home/pgup/pgdn) when opening the dialog and then re-enabling them when closing it. Same for when the search field is focused/unfocused.

From the gtk docs for set_accels_for_action:

To remove all accelerators for an action, use an empty, zero-terminated array for accels.

Perhaps even disable all (or a broader subset at least) of the accels when opening a dialog as the desired behaviour is probably not to be able to use any of them when a dialog is already open (aka at the moment you can bring up the about box, then while it's open, bring up the preferences, through the shortcut keys)

@toolstack
Copy link
Contributor

As for the issues with pgup/pgdn not working when another area (like the sidebar) is focused, it could probably be handled with a check to see what is being focused when the accel fires... aka if its not something like the sidebar, then scroll the post list, but if it is the sidebar, scroll that instead.

@GeopJr
Copy link
Owner Author

GeopJr commented Dec 16, 2024

set_accels_for_action

We can disable individual actions (SimpleAction#set_enabled) but either way this is not the best solution either. I didn't test it but I think any text field would trigger it. Including the entries in the Search and Lists views.

As for the issues with pgup/pgdn not working when another area (like the sidebar) is focused, it could probably be handled with a check to see what is being focused when the accel fires... aka if its not something like the sidebar, then scroll the post list, but if it is the sidebar, scroll that instead.

The way it is right now, they work wherever the focus position is. Which is the desired behavior.

People wanted page up and down to work without being focused in the scroll view.

Also just to point it out, Home works as expected when the focus is in the scroll view. What prompted you to make the initial PR was the desire to have it work outside of it, just like the page up/down ones (rightfully so, since the focus will almost never be in the scrollview if you are using a mouse (unless you click a post)):

Screencast.From.2024-12-16.17-43-02.mp4

notice that the focus is on some post and when I press Home, it goes to the top - without any accels or code from our side.

@toolstack
Copy link
Contributor

We can disable individual actions (SimpleAction#set_enabled) but either way this is not the best solution either. I didn't test it but I think any text field would trigger it. Including the entries in the Search and Lists views.

Agreed, any widget that would normally use the nav keys would be impacted with the problem.

The way it is right now, they work wherever the focus position is. Which is the desired behavior.

Mostly agree, though pgup/dn break inside of the edit field of the compose window as well, so while not as significant of problem as Home, it is still broken in the same way.

Also just to point it out, Home works as expected when the focus is in the scroll view. What prompted you to make the initial PR was the desire to have it work outside of it, just like the page up/down ones (rightfully so, since the focus will almost never be in the scrollview if you are using a mouse (unless you click a post)):

Agreed, pgup/dn and home should have the same functionality overall with whatever solution is decided upon.

I think there are two overall options:

  1. Use the global accels and then disable them were they shouldn't function (like search boxes, text input, dialog boxes, etc)
  2. Don't use the global accels and instead use local keyboard capture on the various widgets where the functionality should be (aka on the containing windows and most common focus areas that don't use the nav keys)

Neither is particularly good but both would get the functionality that I think everyone expects.

@toolstack
Copy link
Contributor

@GeopJr I've thrown together option 1, disabling the global accelerators as needed, over here as a proof of concept, probably needs some thought/changes around if the accels for pgup/dn should still be active in text boxes like the new lists one, but demonstrates a reasonable solution to the issue I think.

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.

[Bug]: Home key used in the editor scrolls the timeline in the main window
2 participants