Skip to content

Allow minimizing app sections#264

Merged
danirabbit merged 12 commits intomasterfrom
shrink-apps
Aug 30, 2023
Merged

Allow minimizing app sections#264
danirabbit merged 12 commits intomasterfrom
shrink-apps

Conversation

@leolost2605
Copy link
Member

@leolost2605 leolost2605 commented Jul 22, 2023

  • Allow to minimize an app section by clicking on the name and/or the triangle icon

Probably fixes #217

Screenshot with a minimized section:
Shrink app sections

@leolost2605 leolost2605 requested a review from a team July 22, 2023 21:28
@vjr
Copy link
Member

vjr commented Jul 23, 2023

I really like this PR 👍🏻 at least the UI/UX/behaviour - I cannot comment on the code for now.

I was trying something similar except using a Hdy.Deck instead of ListBox so the last/latest notification would be visible at the top always and the others would be stacked under the deck (if I'm using proper UI lingo) and clicking the deck would animate the top item move to the bottom, revealing the next in the deck, and so on...

This PR indicator remembers (which I like again) each app's state of whether it is expanded or not every time you close and open the indicator BUT if I log out and log back in ALL apps are in expanded state.

Can we have it remember the state across sessions, or if we don't want to save the states, then make it unexpanded/collapsed by default upon fresh login session for all apps and the other category?

Finally, the expand/collapse animation (which results in the main ListBoxRow increasing/decreasing in height with the scollbar resizing too?) with a bunch of notifications in a couple of apps and an other category seems not so smooth to me, but acceptable. If this can be smoothened further it would be a bonus!

Thank you for this PR!

@leolost2605
Copy link
Member Author

leolost2605 commented Jul 24, 2023

Hey @vjr, thanks for your feedback!

Remembering the expanded state across sessions is now implemented.

Regarding the not so smooth animations: The last commit fixes some flickering that might be happening. If you still have the feeling it could be improved I'd be very happy about a screen record of how it looks for you and some more information about what could need some improvements/what you have in mind it should look like :)

Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

This mostly works but it can crash wingpanel if you "Clear All Notifications" while a section is collapsed.

Terminal output:

ERROR:hashmap.c:3212:gee_hash_map_node_iterator_next: assertion failed: (_stamp == _map._stamp)
Bail out! ERROR:hashmap.c:3212:gee_hash_map_node_iterator_next: assertion failed: (_stamp == _map._stamp)

Another improvement would be to have the section expand/collapse with a click anywhere on the header (apart from the clear button) like Code or Files sidebars. At the moment you have to click on the expander or the text.

@leolost2605
Copy link
Member Author

@jeremypw both should be fixed now.

@leolost2605 leolost2605 requested a review from jeremypw July 25, 2023 17:31
@Marukesu
Copy link
Contributor

one issue with this is that it won't work with AppEntry being a header set with Gtk.ListBoxRow.set_header(). one of the changed needed to make the NotificationList work with GLib.ListModel is making the AppEntry a real header.

@leolost2605
Copy link
Member Author

leolost2605 commented Jul 26, 2023

I did it like this for the smooth animation which I think is not possible using the header_func? Also this would keep compatability with GTK4's ListViews which should fix most of the performance issues with 1000+ notifications.
I think we could still use a ListModel, we just don't bind it directly and do some translation from the notification based model to a app entry based one.

@Marukesu
Copy link
Contributor

I did it like this for the smooth animation which I think is not possible using the header_func? Also this would keep compatability with GTK4's ListViews which should fix most of the performance issues with 1000+ notifications.

Gtk.ListView.header_factory is the replacement of Gtk.ListBox.set_header_func(), so compatibility isn't a issue (given that a Gtk4 port is OS 8 area).

About the animation, you could try to re-use the NotificationEntry internal revealer to make them slide up to the header, hidding one entry per iteration. that would be compatible with set_header_func().

I think we could still use a ListModel, we just don't bind it directly and do some translation from the notification based model to a app entry based one.

think about the ListView compatibility, a multi-level model instead of a flat model would hurt performance, no? since the view won't keep track of the notifications, only of applications, and if a app entry has more notifications than could be seen. they would still be rendered when it won't with the flat model. i believe Gtk.ColumunView (that use a ListView per column) have a performance issue compared to a flat ListView because of that.

@leolost2605
Copy link
Member Author

leolost2605 commented Jul 28, 2023

Gtk.ListView.header_factory is the replacement of Gtk.ListBox.set_header_func(), so compatibility isn't a issue (given that a Gtk4 port is OS 8 area).

Oh nice I didn't know that was coming

think about the ListView compatibility, a multi-level model instead of a flat model would hurt performance, no? since the view won't keep track of the notifications, only of applications, and if a app entry has more notifications than could be seen. they would still be rendered when it won't with the flat model. i believe Gtk.ColumunView (that use a ListView per column) have a performance issue compared to a flat ListView because of that.

I don't really know a lot about gtk performance but shouldn't the individual ListView's for the notifications take care of whether to render their content or not? Also wouldn't that have us resort the whole list everytime a notification gets added while otherwise we would only have to prepend and maybe change the position of an app entry? I'm not sure though whether that might be a big performance hit?

Meanwhile I updated it to not use separate ListBoxes
That changes the animation a bit but I think it still looks smooth :)
Also I didn't do any of the aforementioned GTK4 prep and fix the other issue as now nothing changes surrounding how that gets handled and it's probably best done when changing to header_func

leolost2605 and others added 2 commits July 29, 2023 00:11
Co-authored-by: Gustavo Marques <pushstarttocontinue@outlook.com>
Copy link
Contributor

@Marukesu Marukesu left a comment

Choose a reason for hiding this comment

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

Some minor nits.

I believe there's a question of if we should keep the state even after there's no more notifications from the application, or remove it.

My expectation in this case would be that the state get removed. since the header is removed from the indicator too.

@vjr
Copy link
Member

vjr commented Aug 5, 2023

@leolost2605 I just tried this PR again and I'm happy with it from a user perspective - can't comment on any remaining code or UI/UX review suggestions.

One (final?) request from me, if possible, is can the scrolling be made smooth/kinetic using any readily available UI widgets like from Handy or Gtk4? I've only tried with a mouse on my PC but would be cool if at least with a laptop trackpad this can be achieved?

Cheers!

leolost2605 and others added 2 commits August 5, 2023 12:04
Co-authored-by: Gustavo Marques <pushstarttocontinue@outlook.com>
@leolost2605
Copy link
Member Author

One (final?) request from me, if possible, is can the scrolling be made smooth/kinetic using any readily available UI widgets like from Handy or Gtk4? I've only tried with a mouse on my PC but would be cool if at least with a laptop trackpad this can be achieved?

@vjr AFAIK kinetic scrolling should be enabled by default for touch input. If you want you can test it by enabling "Simulate touchscreen" in the gtk inspector to see whether it's what you had in mind.

I believe there's a question of if we should keep the state even after there's no more notifications from the application, or remove it.
My expectation in this case would be that the state get removed. since the header is removed from the indicator too.

@Marukesu I've got no strong opinion on that, I changed it. Now it's collapsed by default or should it rather be expanded?

@danirabbit
Copy link
Member

I was trying something similar except using a Hdy.Deck instead of ListBox so the last/latest notification would be visible at the top always and the others would be stacked under the deck (if I'm using proper UI lingo) and clicking the deck would animate the top item move to the bottom, revealing the next in the deck, and so on...

This is how iOS handles collapsing as well and I really like that. This way you can still see the latest notification from a given app without it flooding the list. It means you can just always keep a noisy app collapsed and never really have to expand the stack if you don't want to

@leolost2605
Copy link
Member Author

This is how iOS handles collapsing as well and I really like that. This way you can still see the latest notification from a given app without it flooding the list. It means you can just always keep a noisy app collapsed and never really have to expand the stack if you don't want to

Sure I might need some help with CSS stuff though if we want to show like a stack with the other notifications maybe like barely visible under the topmost. Also just to be sure I understand it correctly: we keep the header with the arrow so when clicking it, it will still expand or collapse the notifications from one app. But now we always show a notification and clicking that will cycle through all of them?
Also this will most definitely break compatibility with something like #169 but I'm sure we can work around that.

@danirabbit
Copy link
Member

This is how iOS handles collapsing as well and I really like that. This way you can still see the latest notification from a given app without it flooding the list. It means you can just always keep a noisy app collapsed and never really have to expand the stack if you don't want to

Sure I might need some help with CSS stuff though if we want to show like a stack with the other notifications maybe like barely visible under the topmost. Also just to be sure I understand it correctly: we keep the header with the arrow so when clicking it, it will still expand or collapse the notifications from one app. But now we always show a notification and clicking that will cycle through all of them? Also this will most definitely break compatibility with something like #169 but I'm sure we can work around that.

Thinking about it more, it would be nice just to get the current behavior in and we can think about a more complex behavior later :)

danirabbit and others added 2 commits August 30, 2023 12:49
* Make headers focusable, fix styles, add tooltip

* less specific selector

* use property binding
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'm happy with this, this looks good to me!

@danirabbit danirabbit requested a review from Marukesu August 30, 2023 17:25
Copy link
Contributor

@Marukesu Marukesu left a comment

Choose a reason for hiding this comment

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

LGTM

@danirabbit danirabbit dismissed jeremypw’s stale review August 30, 2023 18:31

Issue was fixed

@danirabbit danirabbit merged commit efae46b into master Aug 30, 2023
@danirabbit danirabbit deleted the shrink-apps branch August 30, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

Group notification to improve handling

5 participants