Skip to content
This repository has been archived by the owner on Nov 14, 2022. It is now read-only.

Update runtime to GNOME 42 #22

Merged
merged 5 commits into from
Jun 27, 2022
Merged

Conversation

cagatay-y
Copy link
Collaborator

Based on #21. I could not figure out a way to suggest the addition of a file to a PR, so I created a separate PR based on it. My reasoning for the patch is as follows.


Trying to build the PR with GNOME 42, the error I got was the following:

../src/Widgets/Status.vala:8.43-8.56: error: construct properties not supported for specified property type
    8 | 	public API.NotificationType? kind { get; construct set; }
      | 	                                         ^~~~~~~~~~~~~~  

Searching the Vala compiler repository, the error seems to be emitted from https://gitlab.gnome.org/GNOME/vala/-/blob/main/vala/valapropertyaccessor.vala#L222. I assume that the first part of the condition is true because the property is marked as construct. For the second part, I checked the is_gobject_property method. My assumption is that in our case the if clause that is taken to return false (and cause the error) is at line 454. When I check the is_gobject_property_type method that is inside the condition, it returns false when the type of the property is an enum (which API.NotificationType is) and it is nullable (marked with the question mark).

Based on https://naaando.gitbooks.io/the-vala-tutorial/content/en/4-object-oriented-programming/gobject-style-construction.html, the constructor set style is for GObject-style construction. I guess (based on the code trail above) the issue is that the type of the property is not compatible with that style. Changing the setting of the property in the constructor to "the Java/C#-style" allowed me to build the code with the version 42 of the GNOME runtime.

@flathubbot
Copy link

Started test build 95847

@flathubbot
Copy link

Build 95847 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/93584/com.github.bleakgrey.tootle.flatpakref

@cagatay-y cagatay-y marked this pull request as draft June 12, 2022 18:13
@cagatay-y
Copy link
Collaborator Author

Converted to draft as it appears that the patch broke deserialization

@harmathy
Copy link
Contributor

@cagatay-y Thanks for diving into this issue! If you manage to fix this, it would also solve bleakgrey/tootle#353.

A nullable cannot be a GObject property. Change the enum property that
caused the error to be non-nullable and add an enum case for what was
normally denoted by null.
@flathubbot
Copy link

Started test build 95876

@cagatay-y cagatay-y marked this pull request as ready for review June 12, 2022 22:53
@flathubbot
Copy link

Build 95876 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/93614/com.github.bleakgrey.tootle.flatpakref

@cagatay-y
Copy link
Collaborator Author

Turns out the deserialization error was a false alarm: The error was also present on the current release on Flathub with version 40. The visible problem was notification type information such as "Reposted by …", "Favorited by …", etc. not being shown. The original workaround of making the property a non-GObject one caused this problem because it broke the property change notifications, which were needed to show the correct notification type.

This patch instead makes the property non-nullable and adds a NONE case to the enum for cases which were previously handled with null.

@cagatay-y cagatay-y marked this pull request as draft June 12, 2022 23:05
The notification type was set to erroneously null and it was interpreted
as the enum case with integer value 0 (the first one).
@flathubbot
Copy link

Started test build 95878

@flathubbot
Copy link

Build 95878 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/93616/com.github.bleakgrey.tootle.flatpakref

@cagatay-y
Copy link
Collaborator Author

I should review more carefully before pushing. Hopefully, the third time is the charm.

@cagatay-y cagatay-y marked this pull request as ready for review June 12, 2022 23:19
@orgcontrib
Copy link

@hfiguiere can you merge this? I can't wait to update my local instance.

@hfiguiere
Copy link
Contributor

@cagatay-y ^^^^

@cagatay-y
Copy link
Collaborator Author

Although I didn't see another problem after the last commit to the PR branch, it would be great to have an additional person review or test the update. I didn't want to merge my own PR.

@hfiguiere
Copy link
Contributor

Merging our own PR is what is expected from the maintainer (you). There is no review process for them, and I surely don't have the bandwidth. But they run CI (which is important).

@cagatay-y
Copy link
Collaborator Author

@harmathy, would you be interested in taking a look?

If there is not any feedback, I will merge tomorrow.

@harmathy
Copy link
Contributor

@cagatay-y, I added your patch to to the AUR package: No issues! Works great! Feel free to merge!

@cagatay-y cagatay-y merged commit 47b404c into flathub:master Jun 27, 2022
@cagatay-y cagatay-y deleted the update-runtime branch June 27, 2022 09:15
@cagatay-y cagatay-y restored the update-runtime branch June 27, 2022 09:17
@cagatay-y cagatay-y deleted the update-runtime branch June 27, 2022 09:17
This was referenced Jul 20, 2022
archlinux-github pushed a commit to archlinux/aur that referenced this pull request Jul 29, 2022
Add patch by Çağatay Yiğit Şahin from
flathub/com.github.bleakgrey.tootle#22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants