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

Constant indicator width #255

Merged
merged 2 commits into from
May 18, 2023
Merged

Constant indicator width #255

merged 2 commits into from
May 18, 2023

Conversation

leolost2605
Copy link
Member

  • Set width_request of NotificationsList to 360 (standard size of a notification)
  • Prevent the indicator from resizing when all notifications are cleared

Before:
Before (trimmed)

After:
After (trimmed)

@leolost2605 leolost2605 requested a review from a team May 14, 2023 21:19
@@ -40,6 +40,7 @@ public class Notifications.NotificationsList : Gtk.ListBox {

activate_on_single_click = true;
selection_mode = Gtk.SelectionMode.NONE;
width_request = 360;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does the value of 360 come from? I notice that in Indicator.vala , get_widget () returns a box with a width_request of 300.

Copy link
Member Author

@leolost2605 leolost2605 May 15, 2023

Choose a reason for hiding this comment

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

Ah right it might be better to change it there then.
360 is usually the width of the NotificationEntries that are the ListBoxRows representing Notifications but I can't say for sure that's always this way. Might this be problematic?

Copy link
Collaborator

@jeremypw jeremypw May 15, 2023

Choose a reason for hiding this comment

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

Yes, changing it to 360 in Indicator.vala has the same effect without adding another line. I am not absolutely sure what limits the width of NotificationEntry - I'll do some testing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like its the width_chars and max_width_chars properties of the title label.

Copy link
Collaborator

Choose a reason for hiding this comment

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

After further investigation, it seems that the elementary notifications server produces a bubble with width 332 (hard coded). So if you want the notification list to match the bubbles, that value should be used in Indicator.vala. Maybe in future that value could be parameterized as a setting so both projects could be easily kept in sync (?)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we use character width here instead of width request so that if the text size changes you don't see fewer characters for large fonts.

It might be worth seeing if we can set a min-width with CSS in characters? Otherwise I wonder if we could pack an invisible fake notification maybe. Not super clean but would make sure it's the same width all the time

Copy link
Member Author

@leolost2605 leolost2605 May 17, 2023

Choose a reason for hiding this comment

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

After further investigation, it seems that the elementary notifications server produces a bubble with width 332 (hard coded). So if you want the notification list to match the bubbles, that value should be used in Indicator.vala. Maybe in future that value could be parameterized as a setting so both projects could be easily kept in sync (?)

The NotificationEntries are actually wider than that (they still look about the same my guess would be that's because of some CSS stuff which i don't really know much about), we probably would have to reduce the width_char then to keep a constant width which doesn't seem optimal.
I would still plead for increasing the width_request in Indicator.vala to 360 as this keeps the width constant most/all of the time (also with bigger text sizes), is very easy and not a very big change as the "default" size is hardcoded already anyway (at 300). The only problem I can see here is that the notifications indicator might take up more screen space which IMHO doesn't do to much harm at 60 units. Another problem is that it's still hardcoded so it might not work anymore when in the future the default sizes of NotificationEntries change.

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 is not perfect but is simple and works in most common situations so I am happy to approve. A more refined solution can be proposed later if required.

@jeremypw jeremypw merged commit b9d30e1 into master May 18, 2023
4 checks passed
@jeremypw jeremypw deleted the constant-size branch May 18, 2023 08:39
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.

None yet

3 participants