Persist conversation list filter switches#1033
Conversation
danirabbit
left a comment
There was a problem hiding this comment.
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
ae7e1df to
e55d92f
Compare
|
Thanks for the guidance, it is effectively cleaner than when bound by hand. Did not know about it. Hope it is better now. |
danirabbit
left a comment
There was a problem hiding this comment.
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!
| SettingsBindFlags.DEFAULT | SettingsBindFlags.SET | ||
| ); | ||
| settings.bind ( | ||
| "hide-unstarred-conversations", | ||
| hide_unstarred_switch, | ||
| "active", | ||
| SettingsBindFlags.DEFAULT | SettingsBindFlags.SET |
There was a problem hiding this comment.
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:
| 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 |
There was a problem hiding this comment.
As I begin with Vala, I still struggle to understand what should be namespaced and what should not. Effectively, this looks cleaner.
| orientation = VERTICAL; | ||
| get_style_context ().add_class (Gtk.STYLE_CLASS_VIEW); | ||
|
|
||
| settings = new GLib.Settings ("io.elementary.mail"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| private void reload_active_folder () { | ||
| if (folder_info_per_account != null) { | ||
| load_folder.begin (folder_info_per_account); | ||
| } | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
| hide_read_switch.toggled.connect (() => load_folder.begin (folder_info_per_account)); | ||
| hide_read_switch.notify["active"].connect (() => reload_active_folder ()); | ||
|
|
||
| hide_unstarred_switch.toggled.connect (() => load_folder.begin (folder_info_per_account)); | ||
| hide_unstarred_switch.notify["active"].connect (() => reload_active_folder ()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
e55d92f to
5f43d57
Compare
danirabbit
left a comment
There was a problem hiding this comment.
Very clean and works great. Nice job!
|
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! |
|
Of course! Thank you for taking the time to implement this feature 😊 |
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: