Skip to content

Group Mullvad Nodes by Country, Scrollable Nodes Section, Exit node subtitle#18

Open
rjocoleman wants to merge 4 commits into
joaophi:masterfrom
rjocoleman:feat/mullvad-country-and-scroll
Open

Group Mullvad Nodes by Country, Scrollable Nodes Section, Exit node subtitle#18
rjocoleman wants to merge 4 commits into
joaophi:masterfrom
rjocoleman:feat/mullvad-country-and-scroll

Conversation

@rjocoleman
Copy link
Copy Markdown
Contributor

More changes in a single commit than I'd like, sorry!

This PR:

  • creates a ScrollablePopupMenu class that supports scrolling and sections with titles.
  • Adds nodes in a PopupMenu.PopupMenuSection to it.
  • If Mullvad nodes are present these are added as a section to the scrollable menu too.
  • in tailscale.js adds the node location
  • Groups Mullvad nodes by country (if more than one node exists in that country)
  • Creates a collapsable menu per country
  • Puts the mullvad exit node at the top of the mullvad nodes list
  • In tailscale.js adds the exit node name (if set) to the client
  • Displays the exit node name as a subtitle on the QuickSettings toggle.

@joaophi
Copy link
Copy Markdown
Owner

joaophi commented Nov 28, 2023

Hi @rjocoleman,

Thanks for you contribution!!

I have a few points I'd like you take a look at:

  • A lot of empty spaces with few nodes
  • Can you disable the hover effect for the whole block, and enabled it for the devices only
  • Keyboard support
    image

You addition to show the exitnode as subtitle I already merged to master a5ce530

I also created a PR(#19) to fix this scroll issue, it's more simple as it simply forces the scrollbar to show in the submenu, so if the items above are hard to achieve we can use as fallback.

@rjocoleman
Copy link
Copy Markdown
Contributor Author

@joaophi I've addessed some of these issues with a new PR on #20

I think its slightly more idiomatic gnome-shell than this PR.

The height of the box is now set dynamically.
The hover styling is a bit better, but due to the way they're nested I couldn't work out the combination to get it quite right.

Keyboard support is problematic. I ended up totally reinventing scrolling a couple of ways and it seems very fragile, so I removed the code. In #20 there is some interaction between scrolling when the mouse is over an open sub menu. I didn't track down that issue yet either.
Both cases, I think there is a more idiomatic way to do some of these things that will lead to less hacking/reimplimentation of primitives but i didn't see it yet.

I have also updated this PR with many of features, but I don't think the approach here is as good as #20, but there are some trade-offs for both.

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.

2 participants