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

RatbagdLed: emit DefaultResolutionChanged on the resolution's object path #226

Merged
merged 1 commit into from
Jul 13, 2017

Conversation

Hjdskes
Copy link
Member

@Hjdskes Hjdskes commented Jul 13, 2017

See libratbag/piper#36 (comment)

The only issue now is that the signal is only emitted on the resolution that has just been set the default. At the very least it should also be emitted on the resolution that was the default, but I don't think this is currently possible so we'll have to go with emitting it on all resolutions. What's the best way to go about this?

(We should probably do this for other signals too.)

@whot whot merged commit f05950f into libratbag:master Jul 13, 2017
@whot
Copy link
Member

whot commented Jul 13, 2017

I merged this because it matches the current spec. But I think we simply need to move these two signals up into the profile.

Or we have a boolean property of "IsDefault" that changes, that way we get to use the PropertyChanged signals as they happen anyway, without the need for extra signals on our behalf. Certainly easier to implement in ratbagd and means we send only two signals whenever the resolution changes (active off and active on). But it means slightly more work on the callers, they now have to listen to all property changed signals and filter.

I don't know, this is probably something we have to implement one way and see if it makes sense, sorry.

@Hjdskes
Copy link
Member Author

Hjdskes commented Jul 14, 2017

I'm not sure which approach I like best.

The latter would mean that each class in Piper that has a Ratbagd* instance (let's say a RatbagdResolution in this example) needs to connect to that instance's notify::is-default signal that is emitted whenever this property changes (automatically, by GObject). This is straightforward, but I'm not sure how well it will work with profile switches.

The former would mean that each class in Piper that has a Ratbagd* instance needs a reference to a profile, so that it can connect to the profile's default-resolution-changed signal (again, just taking the RatbagdResolution example here) and do the work from that callback. This will likely work better with profile switches, albeit being a little less straightforward.

I'm not yet sure which of these will work best once I will implement profile switching: there won't be just one profile to work with but all of them, and the active one can change at any given time. In this case, it is likely that every class in Piper needs a reference to a profile anyway, which will change depending on the active-profile-changed signal emitted from RatbagdDevice (currently RatbagdProfile, but this will have to be changed to prevent the same issue we're having now with RatbagdResolution) to grab, for example, the new profile's resolution to update.

Maybe we should postpone this until I get to implementing profiles so I can experiment a bit and see which approach is best.

@Hjdskes Hjdskes deleted the fix/signals branch July 14, 2017 08:11
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