Skip to content

feat: port to adaptive dialogs #804

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

Merged
merged 18 commits into from
Mar 8, 2024
Merged

feat: port to adaptive dialogs #804

merged 18 commits into from
Mar 8, 2024

Conversation

oscfdezdz
Copy link
Contributor

All remaining dialogs, except for the composer (#623), should be covered by this PR.

Fixes #777

@GeopJr
Copy link
Owner

GeopJr commented Feb 26, 2024

Thank you so much Óscar! 🙇

Don't worry about the CI, I'll deal with it!

@oscfdezdz
Copy link
Contributor Author

Something is wrong, Esc key to close dialogs doesn't work and it should by default with AdwDialog.

@GeopJr
Copy link
Owner

GeopJr commented Feb 26, 2024

(I haven't looked into this PR yet)

Does it happen on all dialogs or some? In some cases we handle it https://github.com/search?q=repo%3AGeopJr%2FTuba+add_binding_action+%28Gdk.Key.Escape&type=code. Anything that is not set to window.close should probably have its AdwDialog's can-close set to false

@oscfdezdz
Copy link
Contributor Author

All, even with Report dialog that has report.back binding and not window.close. It seems like the main window is capturing the shortcut instead, if the active AdwNavigationPage has a back button it would go back when the dialog is open

From what I understand from the migration guide and code, we should not have to explicitly specify it https://gitlab.gnome.org/GNOME/libadwaita/-/blob/main/src/adw-dialog.c?ref_type=heads#L1188-L1189

@GeopJr
Copy link
Owner

GeopJr commented Feb 26, 2024

Yeah looks like the global esc accel is causing it

set_accels_for_action ("app.back", {"<Alt>BackSpace", "<Alt>Left", "Escape", "<Alt>KP_Left", "Pointer_DfltBtnPrev"});

Removing it ("Escape") seems to fix it, though it's still needed :/ Maybe we can check if any dialogs are visible and handle it?

cc: @alice-mkh

@alice-mkh
Copy link
Contributor

Why do you even need that action? Navigation view already has an action for going back AND handles shortcuts

@GeopJr
Copy link
Owner

GeopJr commented Feb 27, 2024

The mediaviewer is an overlay and app.back dismisses it if it's open. I removed the ones navigationview handles and made the mediaviewer dismiss itself on its keypress event instead

@alice-mkh
Copy link
Contributor

Can't you use navigation view for media viewer?

@GeopJr
Copy link
Owner

GeopJr commented Feb 27, 2024

It used to be a stack but changed to an overlay when the scalerevealer was added for the transitions and drag effect

Screencast.from.2024-02-27.13-12-21.webm

@alice-mkh
Copy link
Contributor

alice-mkh commented Feb 27, 2024

I see. Well either way, the way it handles shortcuts is very broken and things would work if you redo it the correct way, using add_binding() etc (on the widget itself, not on the window)

(and make sure you stop handling as needed)

@alice-mkh
Copy link
Contributor

I do want to support transitions like in nav view in future, but yeah, right now it's not possible.

@GeopJr GeopJr merged commit dde9b73 into GeopJr:main Mar 8, 2024
@oscfdezdz oscfdezdz deleted the adw-1-5 branch March 8, 2024 22:48
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.

[Request]: Move most of the dialogs to Adw.Dialog
3 participants