Closed jeremypw closed 3 years ago
I like this a lot more! It seems like a smaller change UX-wise and is really straightforward. I have eventual design changes in the back of my mind that I can work through in a new issue, but I'm happy with this as-is for now.
Yes, I was so used to comboboxes having flat lists that I didn't consider using a TreeStore with them :disappointed: . They still feel a bit "old technology" and using TreeStores and TreeIters is a bit fiddly but this is a much smaller diff. I'll withdraw the other PR and ask for a code review on this one.
@jeremypw I found a weird quirk: due to a weird default resolution, I've added an additional (higher) resolution with xrandr
. This shows up at the bottom of the list instead of the top, and when opening the popover, it is not selected even when it's actually active. I can reproduce this reliably when closing and re-opening the plug in the PR, but not in master (it's still at the bottom of the list in master, but is correctly selected when active).
@cassidyjames I can confirm your issue. At the moment the get_available_modes ()
function is assumed to return the resolutions in descending order. Looks like that is not true for resolutions added with xrandr
. Will have to sort the treestore I guess.
@jeremypw it's not a huge deal as it's certainly an edge case. I recall seeing a reorder function (maybe in another PR?) that orders by total pixel size which I think would also be correct.
@cassidyjames I have now used a Gee.TreeSet to ensure the resolutions are properly unique and sorted. At the moment they are sorted by width and then height. In most cases that is the same as total pixel size but I can easily change the compare func to total pixel size.
@jeremypw ah yeah width then height feels more natural. :+1:
Fixes #178 Fixes #252
A simpler alternative to #301 keeping the existing combobox and using a TreeStore as model.