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

Put recent connections in toplevel applet menu #1652

Merged
merged 3 commits into from
Sep 10, 2022

Conversation

cschramm
Copy link
Member

Other applets like NetworkManager's do not try to keep menu small, so why should blueman...

Closes #1558

This was lying around for a while now. I'm not happy with the presentation yet and don't have any better idea for it but in general this should be good.

@cschramm cschramm requested a review from infirit December 19, 2021 23:59
@infirit
Copy link
Contributor

infirit commented Dec 21, 2021

I tried it and outside of duplicated entires for devices and it being mixed in with the recent connections I worry about the exit menu running off the screen. How are we going to make sure when someone has 24 set as the max recent connections exit is still accessible?

I was thinking of adding an expander when we reach, let's say 6 recent, and hide older ones. This however is going to complicate the current design so yeah.. not sure what's worse.

@infirit
Copy link
Contributor

infirit commented Dec 21, 2021

I'll test with 24 and see how the menu behaves.

@cschramm
Copy link
Member Author

cschramm commented Dec 21, 2021

When I considered #1558 I thought about an adapting expander as well, but came to the conclusion that just adding everything to the top level should be fine. Some arguments for "don't worry about large menus":

  • The default maximum for recent connections is 6.
  • If I remember correctly, back when I wrote the code I looked at nm-applet and found that they have no limit for their menu at all (or least something huge like 50 or something), so they do not seem to worry either.
  • At least GTK-based menu implementations typically show a concept of scrolling, so if all goes wrong, all we get is a scrolling but still usable menu.

I can imagine that it might get rather nasty on low resolutions, though, but I guess at least as people do not increase the default maximum, they should be fine.

Duplicate devices? 🤔 Ah, you mean standard connect vs recent connection items?

@cschramm
Copy link
Member Author

Just checked a very low resolution and indeed multiple status icon menus - including our current one - started scrolling (mate-panel). Also, it looks like I did not remember correctly regarding nm-applet. It shows 5 networks and puts more into a submenu.

In any case, if we talk about adding 5 items on toplevel, plus a submenu item, so that it's up to 6 vs. just adding all items on toplevel that's the same number in 99.9 % of all cases as 6 is exactly our default maximum of recent connections and I don't think it's worth addressing the very special case of an increased maximum and a low resolution just to avoid scrolling in the menu if we're lucky.

@infirit
Copy link
Contributor

infirit commented Dec 22, 2021

Thanks for checking.

This is what I am seeing. The duplicate Devices (Apparaten) was because I used the tray from master instead of this PR. But it still shows up in the middle of all the recently connected devices.
afbeelding

@cschramm
Copy link
Member Author

Ouch, yeah, stupid me. The position indexes start to overlap at some point. 🤦

@cschramm cschramm force-pushed the flatrecentconns branch 2 times, most recently from 2cd4a55 to 7d30555 Compare June 29, 2022 11:11
@cschramm cschramm added this to the 2.4 milestone Jun 29, 2022
@cschramm cschramm marked this pull request as ready for review July 11, 2022 16:54
@infirit
Copy link
Contributor

infirit commented Jul 23, 2022

it's still duplicating entries with the latest changes.
image

Other than that it looks ok to me.

@cschramm
Copy link
Member Author

I cannot replicate any duplication. 🤔

The duplicate Devices (Apparaten) was because I used the tray from master instead of this PR.

again?

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@infirit
Copy link
Contributor

infirit commented Jul 25, 2022

again?

Eeuh, wait, i get deja Vu, i may have done "it" again 😮‍💨

Let me check.

@cschramm cschramm force-pushed the flatrecentconns branch 2 times, most recently from ebfebb7 to 147c27e Compare September 6, 2022 19:29
@infirit
Copy link
Contributor

infirit commented Sep 9, 2022

Can you rebase this one last time :-). I'll make sure to test this weekend.

The item IDs got misused as list indexes there which does not fit blueman-project#1652.
Other applets like NetworkManager's do not try to keep menu small, so why should blueman...

Closes blueman-project#1558
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@infirit infirit left a comment

Choose a reason for hiding this comment

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

Made sure to properly install and restart 😁 . All looks good now.

@cschramm cschramm merged commit 46cd283 into blueman-project:main Sep 10, 2022
cschramm added a commit that referenced this pull request Sep 10, 2022
The item IDs got misused as list indexes there which does not fit #1652.
@cschramm cschramm deleted the flatrecentconns branch September 10, 2022 06:58
cschramm added a commit to cschramm/blueman that referenced this pull request Oct 6, 2022
The item IDs got misused as list indexes there which does not fit blueman-project#1652.
cschramm added a commit that referenced this pull request Oct 6, 2022
The item IDs got misused as list indexes there which does not fit #1652.
cschramm added a commit to cschramm/blueman that referenced this pull request Oct 13, 2022
1297d88 requires this to be available but it is not as it is part of blueman-project#1652. This backports the relevant changes.
cschramm added a commit that referenced this pull request Oct 14, 2022
1297d88 requires this to be available but it is not as it is part of #1652. This backports the relevant changes.
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.

UI improvements suggestions
2 participants