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

XdpAppInfo refactoring #1623

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft

Conversation

swick
Copy link
Collaborator

@swick swick commented Feb 11, 2025

Contains a few commits from #1619.

  • use the GInitable interface with pid and pidfd to create the various XdpAppInfo subclasses
  • make registration a construction property for XdpAppInfoHost
  • make testing XdpAppInfo subclasses possible by adding a testing construction property and creating mock objects based on new environment variables
  • drop XdpAppInfoTest
  • add new fixtures to control the exact XdpAppInfo the portal frontend will detected

Still a bit rough and needs some cleaning up, but I think the end result is a good improvement.

@swick swick force-pushed the wip/app-info-initable branch from 8c4eb49 to 3511b2d Compare February 11, 2025 20:46
@GeorgesStavracas
Copy link
Member

The "mess" the I mentioned in my PR is having to add private setters for private fields of the parent class. I ended up with similar diff and I still dislike it. Which is why I opted for the splitting up the properties, and handling the construction complexity in the xdp_app_info_*_new functions. This is similar to various internal constructors in GTK.

I like the tests this PR introduces, but I don't think the XdpAppInfo changes are worth it.

@swick
Copy link
Collaborator Author

swick commented Feb 12, 2025

At least when using pid and pidfd as construction properties, and then using setters in the initable_init, we get uniform construction via initable_new.

When using xdp_app_info_*_new to set different construction properties, what purpose do the properties serve? Compared with using setters as is done in this MR, it seems like a more roundabout way of setting the parent private data.

@swick
Copy link
Collaborator Author

swick commented Feb 12, 2025

Tried to minimize the changes to get the improved XdpAppInfo testability #1627

@jadahl
Copy link
Collaborator

jadahl commented Feb 13, 2025

When using xdp_app_info_*_new to set different construction properties, what purpose do the properties serve? Compared with using setters as is done in this MR, it seems like a more roundabout way of setting the parent private data.

Using construct properties is a common pattern used in GObject code bases to plumb things up the type chain without having to add private setters.

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.

3 participants