Migrate to new libadwaita Adaptive Dialogs #95

sungsphinx · 2024-02-03T21:48:28Z

Relevant: https://gnome.pages.gitlab.gnome.org/libadwaita/doc/main/migrating-to-adaptive-dialogs.html

2nd attempt, hopefully good for review.

Changes Here: https://github.com/vixalien/muzika/pull/95.diff

vixalien

Can you rename this to "feat: change window min height and width"

And please say why this is necessary in the commit message.

sungsphinx · 2024-02-03T22:18:11Z

Can you rename this to "feat: change window min height and width"

And please say why this is necessary in the commit message.

Done (hopefully)

@vixalien

vixalien · 2024-02-04T11:48:24Z

From the Human Interface Design Guidelines:

Apps that are appropriate for a phone form factor should scale down to 360×294px.

https://developer.gnome.org/hig/guidelines/adaptive.html

sungsphinx · 2024-02-04T12:08:42Z

From the Human Interface Design Guidelines:

Apps that are appropriate for a phone form factor should scale down to 360×294px.

https://developer.gnome.org/hig/guidelines/adaptive.html

Screencast.from.2024-02-04.12-05-13.webm

vixalien · 2024-02-04T13:49:51Z

Those errors are possibly from other screens having a conflicting default height. I'd recommend looking into that instead..

sungsphinx · 2024-02-04T13:51:02Z

Those errors are possibly from other screens having a conflicting default height. I'd recommend looking into that instead..

so should I change the height to the HIG one then?

it seems to break and not show some stuff at the bottom when that small.

vixalien · 2024-02-04T14:00:56Z

Yes. Will need to investigate further.

sungsphinx · 2024-02-04T14:01:30Z

Yes. Will need to investigate further.

Done, is this good for a re-review now? @vixalien

sungsphinx · 2024-02-04T19:50:40Z

@vixalien looks better now, I also saw some suggested changes that disappeared, should I do those?

vixalien · 2024-02-04T19:40:49Z

src/application.ts

@@ -140,4 +140,4 @@ export class Application extends Adw.Application {

 export function get_player() {
  return (Gtk.Application.get_default() as Application).player;
-}
+}


can you please re-add the whitespace at the end of this file?

make sure to do so in this same commit (i.e don't add another fixup commit)

You didn't resolve this either..

@vixalien must have not pushed correctly... Could you fix these?

Yeah, git can get finnicky sometimes. I will try to fix these tomorrow. Have a great night.

Yeah, git can get finnicky sometimes. I will try to fix these tomorrow. Have a great night.

Thanks, you too!

src/application.ts

vixalien · 2024-02-04T19:41:38Z

src/application.ts

-      this.preferences_window.set_transient_for(this.get_active_window());
+    if (!this.preferences_dialog) {
+      this.preferences_dialog = new MuzikaPreferencesDialog();
+      //this.preferences_dialog.set_transient_for(this.get_active_window());


remove this comment as well

I think we can mark this as resolved now.

data/ui/pages/preferences.blp

vixalien · 2024-02-04T20:14:58Z

src/pages/playlist.ts

@@ -170,7 +170,6 @@ export class PlaylistPage extends Adw.Bin
    if (this.playlist?.editable !== true) return;

    const dialog = Adw.MessageDialog.new(


Should this be an Adw.AlertDialog?

You didn't actually change this..

be careful when you mark conversations as resolved.

Weird... I did change it...

src/components/playlist/get_add_to_playlist.ts

data/ui/components/playlist/edit.blp

src/pages/playlist.ts

vixalien · 2024-02-04T20:22:05Z

Whew... finaly reviewed this. Check the requested changes, and don't hesitate to ask me in case of doubt.

vixalien · 2024-02-04T20:23:23Z

@vixalien looks better now, I also saw some suggested changes that disappeared, should I do those?

Oops sorry... I was trying to do a review, but failed. You can see the suggested changes again now.

Also, you can review the commits I made and let me know if all is okay.

vixalien · 2024-02-04T20:24:28Z

Note to self: reorder commits before merging

sungsphinx · 2024-02-04T20:59:00Z

@vixalien re-review? 😅

sungsphinx · 2024-02-04T21:04:27Z

Also, you can review the commits I made and let me know if all is okay.

They seem fine, no problems here!

vixalien · 2024-02-04T21:24:45Z

src/window.ts

-    page.set_modal(true);
-    page.set_transient_for(this);
-    page.present();
+    //page.set_modal(true);


remove these comments too

vixalien · 2024-02-04T21:25:20Z

data/ui/components/playlist/edit.blp

-  default-width: 360;
-  default-height: 440;
+  content-width: 360;
+  content-height: 440;


remove the content-height, it's unnecessary

vixalien · 2024-02-04T21:29:35Z

please pull this branch beforehand, as I just made some changes

sungsphinx · 2024-02-04T21:31:54Z

Hi, I'm currently not at my computer right now, so I can't do too much.

vixalien · 2024-02-04T21:37:29Z

Hey there! I see a bit of the changes I requested were just marked as resolved without first implementing them, which is problematic. A few of the comments I requested to be removed are still there, as well as some newlines at the end of the file..

Please review each change request carefully and update the relevant commit (you don't need to add extra commits) and re-request a review when done. Thanks and have a great day.

sungsphinx · 2024-02-04T21:39:46Z

Hey there! I see a bit of the changes I requested were just marked as resolved without first implementing them, which is problematic. A few of the comments I requested to be removed are still there, as well as some newlines at the end of the file..

Please review each change request carefully and update the relevant commit (you don't need to add extra commits) and re-request a review when done. Thanks and have a great day.

I think something weird happened with git, I did those changes and pushed...

vixalien · 2024-02-04T21:43:57Z

Then I think there might be an issue on my end. Let me recheck

Reason for width change: bottom sheets cut off to the right with the width under 360 Reason for height change: be in line with the GNOME HIG (https://developer.gnome.org/hig/guidelines/adaptive.html#small-size-handling)

vixalien · 2024-02-05T13:23:43Z

Okay! I fixed the various issues and used the Adw.AlertDialog.choose API. I would appreciate if you would test and review this before I merge it.

When you are in the normal browsing mode (i.e. the now player view is not open) the now player view woud still request 294px, however the miniplayer is already visible which caused overflow.

sungsphinx · 2024-02-05T15:56:55Z

Okay! I fixed the various issues and used the Adw.AlertDialog.choose API. I would appreciate if you would test and review this before I merge it.

Thank you, will do.

sungsphinx · 2024-02-05T16:14:35Z

Okay! I fixed the various issues and used the Adw.AlertDialog.choose API. I would appreciate if you would test and review this before I merge it.

Thank you, will do.

Playlists seem to work fine (also tested the delete dialog, the playlist doesn't disappear, this might just be an unrelated issue though).

Edit Dialog:

About Dialog:

Preferences Dialog:

Will update if I encounter anything.

@vixalien

sungsphinx · 2024-02-05T17:20:49Z

Is this ready yet?

vixalien · 2024-02-05T17:55:05Z

Yes it's ready.

vixalien reviewed Feb 3, 2024

View reviewed changes

sungsphinx force-pushed the sungsphinx/adw-dialog-v2 branch from a5b3aef to 93c5dac Compare February 3, 2024 22:17

sungsphinx requested a review from vixalien February 3, 2024 22:18

sungsphinx force-pushed the sungsphinx/adw-dialog-v2 branch from 93c5dac to e7f0ea7 Compare February 4, 2024 12:14

sungsphinx force-pushed the sungsphinx/adw-dialog-v2 branch from e7f0ea7 to 30ae5e8 Compare February 4, 2024 14:00

vixalien requested changes Feb 4, 2024

View reviewed changes

sungsphinx force-pushed the sungsphinx/adw-dialog-v2 branch from 042de74 to fbd0510 Compare February 4, 2024 20:55

sungsphinx requested a review from vixalien February 4, 2024 20:58

vixalien requested changes Feb 4, 2024

View reviewed changes

vixalien force-pushed the sungsphinx/adw-dialog-v2 branch from fbd0510 to 644fc3b Compare February 4, 2024 21:29

vixalien marked this pull request as draft February 4, 2024 21:37

vixalien changed the title ~~[2nd Take] feat: Port Dialogs and Popup Windows to Adw.Dialog~~ Migrate to new libadwaita Adaptive Dialogs Feb 4, 2024

vixalien mentioned this pull request Feb 4, 2024

v1 #30

Open

12 tasks

feat: Change window min height and width

978b7d9

Reason for width change: bottom sheets cut off to the right with the width under 360 Reason for height change: be in line with the GNOME HIG (https://developer.gnome.org/hig/guidelines/adaptive.html#small-size-handling)

vixalien force-pushed the sungsphinx/adw-dialog-v2 branch 3 times, most recently from f1694f3 to fa34c5d Compare February 5, 2024 13:22

sungsphinx and others added 6 commits February 5, 2024 15:26

feat: Port about window to Adw.Dialog

0dc4a54

feat: Port preferences window to Adw.Dialog

7cc54c2

feat: make Login Dialog adaptive

eee7925

feat: Port Adw.MessageDialogs to Adw.AlertDialog

ec86ae2

feat: Port Edit Playlist dialog to Adw.Dialog

0bd02ac

feat: make the Now Playing view adaptive

0afd20d

When you are in the normal browsing mode (i.e. the now player view is not open) the now player view woud still request 294px, however the miniplayer is already visible which caused overflow.

vixalien force-pushed the sungsphinx/adw-dialog-v2 branch from fa34c5d to 0afd20d Compare February 5, 2024 13:33

vixalien marked this pull request as ready for review February 5, 2024 13:34

vixalien approved these changes Feb 5, 2024

View reviewed changes

vixalien merged commit ea1e02e into vixalien:main Feb 5, 2024
2 checks passed

		@@ -170,7 +170,6 @@ export class PlaylistPage extends Adw.Bin
		if (this.playlist?.editable !== true) return;

		const dialog = Adw.MessageDialog.new(

Migrate to new libadwaita Adaptive Dialogs #95

Migrate to new libadwaita Adaptive Dialogs #95

Conversation

sungsphinx commented Feb 3, 2024 • edited by vixalien Loading

vixalien left a comment

Choose a reason for hiding this comment

sungsphinx commented Feb 3, 2024 • edited Loading

vixalien commented Feb 4, 2024

sungsphinx commented Feb 4, 2024

vixalien commented Feb 4, 2024

sungsphinx commented Feb 4, 2024 • edited Loading

vixalien commented Feb 4, 2024

sungsphinx commented Feb 4, 2024 • edited Loading

sungsphinx commented Feb 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vixalien commented Feb 4, 2024

vixalien commented Feb 4, 2024

vixalien commented Feb 4, 2024

sungsphinx commented Feb 4, 2024

sungsphinx commented Feb 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vixalien commented Feb 4, 2024

sungsphinx commented Feb 4, 2024

vixalien commented Feb 4, 2024

sungsphinx commented Feb 4, 2024

vixalien commented Feb 4, 2024

vixalien commented Feb 5, 2024

sungsphinx commented Feb 5, 2024

sungsphinx commented Feb 5, 2024

sungsphinx commented Feb 5, 2024 • edited Loading

vixalien commented Feb 5, 2024

sungsphinx commented Feb 3, 2024 •

edited by vixalien

Loading

sungsphinx commented Feb 3, 2024 •

edited

Loading

sungsphinx commented Feb 4, 2024 •

edited

Loading

sungsphinx commented Feb 4, 2024 •

edited

Loading

sungsphinx commented Feb 5, 2024 •

edited

Loading