Persist conversation list filter switches

[lobre]

Hey, don't know if the code is correct enough, as I am trying to find my way with gschema.

This is an answer to #1032.

I have tested with:

meson compile -C build
glib-compile-schemas build/data
GSETTINGS_SCHEMA_DIR=$PWD/build/data ./build/src/io.elementary.mail

[danirabbit]

Hey thanks for your branch! You're definitely on the right track here. I would recommend using Settings.bind instead though: https://valadoc.org/gio-2.0/GLib.Settings.bind.html

[lobre]

Thanks for the guidance, it is effectively cleaner than when bound by hand. Did not know about it.

Hope it is better now.

[danirabbit]

Nice, that's much cleaner! Left a few more inline comments. This does work as expected, so once those are cleaned up happy to approve!

[danirabbit]

DEFAULT is GET | SET, so you don't need to also include SET: https://valadoc.org/gio-2.0/GLib.SettingsBindFlags.DEFAULT.html

You can also omit the namespace for BindFlags here:

Suggested change

	SettingsBindFlags.DEFAULT | SettingsBindFlags.SET
	);
	settings.bind (
	"hide-unstarred-conversations",
	hide_unstarred_switch,
	"active",
	SettingsBindFlags.DEFAULT | SettingsBindFlags.SET
	DEFAULT
	);
	settings.bind (
	"hide-unstarred-conversations",
	hide_unstarred_switch,
	"active",
	DEFAULT

[lobre]

As I begin with Vala, I still struggle to understand what should be namespaced and what should not. Effectively, this looks cleaner.

[danirabbit]

If possible its usually nicer to group thing together. So instantiating this settings object and the two bindings can all be organize together.

It would also be nice to move them below widget creation where the other signals are created. So that would become like line 205ish. That way everything to do with these options is all together nice and tidy for the next person

[lobre]

I have left the widgets creation where they were, and moved the binding of settings and signals to the end of the function, close together. I hope this is what you expected.

[danirabbit]

It looks like load_folder does intentionally take a nullable here. If we need to add a null check it should be in that function and we should change the params to not accept a nullable and add requires to the function instead of adding a new separate function

[lobre]

I am not sure I fully understood your comment. So I did not know if I had to do the null check from inside load_folder, by just skipping when the parameter was null (as in my helper), or if I had to simply add a requires guard to say that folder_info_per_account should never be null.

I went with the latter, but not fully knowing about what would happen if this function were called somewhere with null as the value. Would it panic at runtime?

I added that helper initially because I had the feeling that there were possibilities of null, especially at the application start when no folder has been selected yet. But maybe it is a bad reading, and that situation cannot happen (and now if it does, at least it will yield an error).

I would appreciate a confirmation here, to see if I understood you and the code correctly!

[danirabbit]

I misread it, but I think you did the right thing here! The requires makes sense to me. It doesn't look like the docs make it explicit, but I believe its the same as warn if fail: https://docs.vala.dev/tutorials/programming-language/main/04-00-advanced-features/04-01-assertions-and-contract-programming.html#_4-1-assertions-and-contract-programming

[danirabbit]

I don't love to see refactoring changes also mixed in with feature changes unless its absolutely necessary. Is there a reason you changed this from connecting to toggled to notifying on the active property?

[lobre]

I think I originally switched from toggled to notify["active"] while experimenting with manual persistence, before switching to Settings.bind. But this may not be needed now, effectively.

[danirabbit]

Very clean and works great. Nice job!

[lobre]

Just wanted to say thanks for taking the time with qualitative reviews and helping to ramp up, as I am still in my early contributions to the eOS ecosystem (and Vala in general).

It is a pleasure to take part in a community that is warm and welcoming to newcomers. Kudos and thank you for merging this!

[danirabbit]

Of course! Thank you for taking the time to implement this feature 😊