Tails

#8 Updated by intrigeri 2019-10-03 06:01:04

Hi @touss!

First, thanks a lot for your first (right?) contribution to Tails!
In particular, we severely lack folks who feel comfortable going closer than 1km from our Perl code, so I’m very happy you’re up to it. If you would like to do more work in this area, just say so, I can easily point you to some next candidate tasks :)

I have only one question about your branch: is there any specific reason why you’ve used Gtk3::Box->new("horizontal", 0) instead of the higher-level Gtk3::HBox class?

Building on top of your work, I’ve done a little bit of cleanup (mostly code style and indentation):
https://git.tails.boum.org/persistence-setup/log/?h=bugfix/15102-have-a-show-passphrase-option-when-creating-persistence
I would be glad if you could review these small changes of mine.

Meanwhile, I’m going to prepare a branch in tails.git that applies these changes, so I can build a Tails image with them and run our automated test suite on it.

#12 Updated by touss 2019-10-04 03:19:24

intrigeri wrote:
> Hi touss! > > First, thanks a lot for your first (right?) contribution to Tails! > In particular, we severely lack folks who feel comfortable going closer than 1km from our Perl code, so I'm very happy you're up to it. If you would like to do more work in this area, just say so, I can easily point you to some next candidate tasks :) > > I have only one question about your branch: is there any specific reason why you've used Gtk3::Box->new(“horizontal”, 0)@ instead of the higher-level Gtk3::HBox class?
>
> Building on top of your work, I’ve done a little bit of cleanup (mostly code style and indentation):
> https://git.tails.boum.org/persistence-setup/log/?h=bugfix/15102-have-a-show-passphrase-option-when-creating-persistence
> I would be glad if you could review these small changes of mine.
>
> Meanwhile, I’m going to prepare a branch in tails.git that applies these changes, so I can build a Tails image with them and run our automated test suite on it.

Hello, thanks for reviewing this code.
This is, in fact, my first conintrigeri wrote:
> Hi touss! > > First, thanks a lot for your first (right?) contribution to Tails! > In particular, we severely lack folks who feel comfortable going closer than 1km from our Perl code, so I'm very happy you're up to it. If you would like to do more work in this area, just say so, I can easily point you to some next candidate tasks :) > > I have only one question about your branch: is there any specific reason why you've used Gtk3::Box->new(“horizontal”, 0)@ instead of the higher-level Gtk3::HBox class?
>
> Building on top of your work, I’ve done a little bit of cleanup (mostly code style and indentation):
> https://git.tails.boum.org/persistence-setup/log/?h=bugfix/15102-have-a-show-passphrase-option-when-creating-persistence
> I would be glad if you could review these small changes of mine.
>
> Meanwhile, I’m going to prepare a branch in tails.git that applies these changes, so I can build a Tails image with them and run our automated test suite on it.

Hello, thanks for reviewing this code.
This is, in fact, my first contribution writing code for Free Software. it’s been a very rich experience so far. I’m learning Perl, Git and GTK3 so that i can implement these tasks.
I would like to continue working on this area, so please point me to other tasks you have in mind.
There’s no specific reason that i chose to use this code, should i change to Gtk3::HBox class to keep the code consistent?
I reviewed the code, what i should do to confirm the changes?

#14 Updated by touss 2019-10-05 00:55:42

Sorry, I just saw in the gtk+ reference manual that the GtkHBox has been deprecated:[https://developer.gnome.org/gtk3/stable/GtkHBox.html]
Should i keep Gtk3::Box->new("horizontal", 0) instead of the higher-level Gtk3::HBox@ class?

#15 Updated by intrigeri 2019-10-05 04:34:31

Hi @touss!

> Sorry, I just saw in the gtk+ reference manual that the GtkHBox have been deprecated:[https://developer.gnome.org/gtk3/stable/GtkHBox.html]

Oh, good catch, I was not aware of it. Thanks!

> Should i keep Gtk3::Box->new("horizontal", 0) instead of the higher-level Gtk3::HBox@ class?

Yes, you’re absolutely right.

But then, I think we should pass the name of the orientation property ('GTK_ORIENTATION_HORIZONTAL'), which is public API, instead of its value ("horizontal"), which is a private implementation detail of GTK: https://developer.gnome.org/gtk3/stable/gtk3-Standard-Enumerations.html#GtkOrientation.

We already have an example of using a member of the GtkOrientation enumeration for similar purposes:

lib/Tails/Persistence/Step/Configure.pm:        $list_box->insert(Gtk3::Separator->new('GTK_ORIENTATION_HORIZONTAL'), -1);

Do you prefer doing this yourself (might give you a greater feeling of accomplishment, I don’t know) or should I?

#16 Updated by touss 2019-10-05 17:07:16

Hello @intrigeri,
> Do you prefer doing this yourself (might give you a greater feeling of accomplishment, I don’t know) or should I?

If that’s ok, i prefer that you do it.

#17 Updated by touss 2019-10-07 04:17:24

> But then, I think we should pass the name of the orientation property (‘GTK_ORIENTATION_HORIZONTAL’), which is public API, instead of its value (“horizontal”)
I made this fix.Please review.

#21 Updated by intrigeri 2019-10-07 15:10:30

Hi @touss,

great, thanks! I’ve merged your branch into the master branch of persistence-setup.git and updated the corresponding topic branch in tails.git. If our CI is happy with it, I’ll merge this and it’ll make it into Tails 4.0~rc1. Congrats!

To save us some overhead, I’ve sneaked an extra, unrelated commit on top: https://git-tails.immerda.ch/persistence-setup/commit/?h=bugfix/15102-have-a-show-passphrase-option-when-creating-persistence and I would appreciate if you could review it. Possibly relevant info that might help:

• The rationale for this change comes from Bug #10976#note-67. I don’t expect you to read that entire ticket. Let’s just say that for years, we’ve been struggling to understand that bug, we’ve done a few changes that may improve the robustness of the “saving a new persistence.conf file”, and it’s not even clear whether they’re sufficient and whether the bug is in persistence-setup itself. So this commit is merely yet another attempt at making this operation more robust. I don’t know if it will make a difference. I’m reasonably convinced it won’t do harm.
• The config_file_path and backup_config_file_path attributes have type AbsPath, which comes from Types::Path::Tiny, that is they’re effectively objects of the Path::Tiny class (with some additional checks that Types::Path::Tiny performs when setting a new value). So we can call https://metacpan.org/pod/Path::Tiny#parent on them.
• I know that not everyone is comfortable with Modern Perl object oriented programming and it’s easy to get lost in all the options. This program is using Moo.

#23 Updated by touss 2019-10-08 02:45:29

> great, thanks! I’ve merged your branch into the master branch of persistence-setup.git and updated the corresponding topic branch in tails.git. If our CI is happy with it, I’ll merge this and it’ll make it into Tails 4.0~rc1. Congrats!
I am very happy to have made my first contribution to Tails. I’ve been meaning to contribute to a free software project for a while and it turns out that Tails has the best documentation for me.
thank you very much @intrigeri to guide me and for all the feedback you gave me. I expect to be able to do more work on Tails.

> To save us some overhead, I’ve sneaked an extra, unrelated commit on top: https://git-tails.immerda.ch/persistence-setup/commit/?h=bugfix/15102-have-a-show-passphrase-option-when-creating-persistence and I would appreciate if you could review it. Possibly relevant info that might help:
Thanks for pointing this out to me, it’s very interesting! I’ll investigate further and try to learn more about it.