refactor: ported upower,volume,networkmanager modules to gtk4 #1029
Conversation
| @@ -274,7 +275,7 @@ impl Module<Button> for UpowerModule { | |||
| label.set_label_escaped(&format); | |||
| }); | |||
|
|
|||
| container.show_all(); | |||
| container.show(); | |||
There was a problem hiding this comment.
You should be able to remove this as widgets are shown by default in gtk4
src/modules/upower.rs
Outdated
| @@ -177,7 +177,8 @@ impl Module<Button> for UpowerModule { | |||
| let label = Label::builder() | |||
| .label(&self.format) | |||
| .use_markup(true) | |||
| .angle(self.layout.angle(info)) | |||
| // TODO: find replacement | |||
There was a problem hiding this comment.
I've not tested but angle should be settable using CSS in gtk4
There was a problem hiding this comment.
Hi, yep I was searching and it should, will remove it.
|
|
||
| { | ||
| let tx = tx.clone(); | ||
| let selector = sink_selector.clone(); | ||
|
|
||
| slider.connect_button_release_event(move |scale, _| { | ||
| slider.connect_value_changed(move |scale| { |
There was a problem hiding this comment.
Changing the event type here could cause issues - unless debounced, this can overload the pulseaudio client and cause it to crash
There was a problem hiding this comment.
Oh yep, just tried and crashed .... I could not find the other event in gtk4, will look deeper, any suggestion are welcome
There was a problem hiding this comment.
The event is still there, it's just been moved to the GtkGestureClick controller struct you have to create and attach. You'll find examples of that elsewhere in the port.
There was a problem hiding this comment.
Hi,
I tried and could not make it work, I though than to try another approach that was to rate limits the events, and seems to work ok, is a bit longer code but on the other side you can direct feedback on the volume level while you scroll it here the impl detail: d632d4b#diff-8def1800cfd70c672062713f20a02cd1af7d852bc2058887066768d171ba6698R195 the key part is the sleep(30) which is not really perceptible from the users but seems to save from a crash (at least in my tests)
There was a problem hiding this comment.
What issues did you run into that meant you couldn't get it to work? Something GTK4 specific?
This approach may work but it runs the risk that 30ms isn't enough on some machines, or that if events aren't processed fast enough (due to the delay) you'll let go of the slider and watch it partially "replay" the drag.
There was a problem hiding this comment.
The issue I had is that I created the GestureClick connected the release event and add it as controller to the scale, but no event was fired when I switched the volume, will try to redo it and post it here.
Yes I thought the same 30ms may not be enough in some slow machines it could be increased probably up to 100ms without user feeling the gap(or made it configurable). anyway this patch actually send one event every 30ms and drop all the event that happen in the middle, so it won't replay the drag it will jump between states.
There was a problem hiding this comment.
Actually do you know if is this is a reported bug in pulse audio, it may be worth to report it upstream.
There was a problem hiding this comment.
but no event was fired when I switched the volume, will try to redo it and post it here.
Strange, deffo share when you can. As far as I'm aware in GTK4 everything gets all events by default.
this patch actually send one event every 30ms and drop all the event that happen in the middle, so it won't replay the drag it will jump between states.
Ah cool, that might work okay then. If option A is a dead end, I'll give it a test when I can.
do you know if is this is a reported bug in pulse audio
No idea. I think I did look before, but couldn't find anything. Pulse documentation is basically non-existent, so it could be that I'm doing something wrong and just don't know.
There was a problem hiding this comment.
added this commit that comment the delay implementation and use the connect_release event, but it does not work, so you can double check it.
src/modules/volume.rs
Outdated
| item_container.append(&label); | ||
| item_container.append(&slider); | ||
| item_container.append(&btn_mute); | ||
| item_container.show(); |
tglman
changed the title
|
Hi, I also added a commit that start the migration of the tray module, as now there are no compilation error, and the tray show with a new menu based on PopoverMenu, anyway I did not yet connect the actions, and also the UI seems a bit messy in my tests. I have to discover yet how to connect the actions (I saw example using SimpleActionGroup) and how to compute the required parameters of Client.activate |
|
Really appreciate the undertaking there. Could you move the tray port into a separate PR please because that's gonna be a big one. It'll make it easier to maintain and hopefully means I can get the other ports merged first. Once that's picked across and removed here I'll give this another review and see where we're at. |
|
Hi, Yes it does make sense to move the tray to a different PR, done it, with this commit: 13336e0 this changes try to do change the volume with the mouse released event, but it does not work, removing it, the changes will work with the value change event and the "rate limitated" event propagation. I'm actually feeling a bit sorry for stealing all the fun of the migration :) |
|
Perfect cheers, I'll try and review this evening if I can.
I've certainly had more fun :P I'm very glad to have somebody else helping out, so seriously a huge thanks! |
tglman
changed the title
Hi,
This migrate the volume and upower modules to gtk4