flathub / com.github.bleakgrey.tootle

https://flathub.org/apps/details/com.github.bleakgrey.tootle
4 stars 4 forks source link

Update runtime to GNOME 42 #22

Closed cagatay-y closed 2 years ago

cagatay-y commented 2 years ago

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 commented 2 years ago

Started test build 95847

flathubbot commented 2 years ago

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 commented 2 years ago

Converted to draft as it appears that the patch broke deserialization

harmathy commented 2 years ago

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

flathubbot commented 2 years ago

Started test build 95876

flathubbot commented 2 years ago

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 commented 2 years ago

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.

flathubbot commented 2 years ago

Started test build 95878

flathubbot commented 2 years ago

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 commented 2 years ago

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

orgcontrib commented 2 years ago

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

hfiguiere commented 2 years ago

@cagatay-y ^^^^

cagatay-y commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

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

harmathy commented 2 years ago

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