Tails

#1 Updated by segfault 2017-12-12 10:37:28

• Deliverable for deleted (299)

Actually this is not something we have to deliver as part of our SponsorW work, so I guess I shouldn’t flag it as such.

#2 Updated by sajolida 2017-12-17 20:36:34

• Description updated

#3 Updated by segfault 2018-01-23 10:36:04

• Priority changed from Low to Normal
• Target version changed from Tails_3.8 to Tails_3.9
• Deliverable for set to 299

#4 Updated by segfault 2018-01-23 10:36:29

• blocked by Feature #15227: VeraCrypt iteration 3: Support creating and modifying VeraCrypt volumes in GNOME added

#5 Updated by segfault 2018-01-25 16:05:34

• Subject changed from Create VeraCrypt Mounter application to Iteration 4: Create VeraCrypt Mounter application

#7 Updated by intrigeri 2018-06-03 12:54:03

• Target version deleted (Tails_3.9)

Let’s set a target version again only if we fail to implement & upstream the better, desired UX.

#8 Updated by sajolida 2018-06-04 16:08:24

• Assignee changed from segfault to intrigeri
• QA Check set to Info Needed

> Let’s set a target version again only if we fail to implement & upstream the better, desired UX.

I don’t understand that comment… What do you mean by “the better, desired UX”?

VeraCrypt Mounter is about making it easier for users to discover and learn how to use VeraCrypt in Tails. Nothing from what we designed elsewhere in the this project could ever replace that…

#9 Updated by sajolida 2018-06-04 19:25:44

• related to Feature #15628: Consider re-enabling automounting to improve VeraCrypt UX added

#11 Updated by intrigeri 2018-06-05 10:30:41

• Assignee changed from intrigeri to segfault
• QA Check deleted (Info Needed)

#18 Updated by intrigeri 2018-06-11 13:44:07

• blocks deleted (Feature #15227: VeraCrypt iteration 3: Support creating and modifying VeraCrypt volumes in GNOME)

#19 Updated by intrigeri 2018-06-11 13:45:33

• Target version set to Tails_3.9

#20 Updated by intrigeri 2018-06-11 13:46:06

• blocked by Feature #15223: Iteration 2: Support unlocking VeraCrypt file containers in GNOME added

#21 Updated by intrigeri 2018-06-11 14:36:35

• blocks Feature #14476: Write user documentation for VeraCrypt support in Tails added

#23 Updated by intrigeri 2018-07-04 09:32:41

• Status changed from Confirmed to In Progress
• Feature Branch set to segfault:feature/14481-TCRYPT-support-beta

I had a first quick look as part of Feature #14481. I did not review the VeraCrypt Mounter codebase itself yet and I did not test it. So I’m only commenting about the plumbing:

• It’s a bit annoying to drop stuff directly into config/chroot_local-includes/usr/local/lib/python3.5/dist-packages: for example, that won’t work on Buster, so once this is merged into devel and then into feature/buster, it’ll be a bit painful to maintain two copies living in different places. VeraCrypt Mounter is a big enough project to warrant its own Git repo. Maybe it’s not worth going through the whole “custom Debian package” thing at least for now, so I would use a Git submodule which seems like a good trade-off (not too much overhead, but still way cleaner). But then we’ll need to add this new repo to the Transifex setup and integrate this into the release process, so that’s perhaps too much overhead still. So well, maybe find a way to track this in tails.git but without hard-coding OS release -specific Python version in the path?
• commit:9523245b705564e3d95832642711ce4dc617cf2b feels weird: I doubt config/chroot_local-includes/usr/share/veracrypt-mounter/ui/*.ui belong to a list of POT files; if this is going to stay in tails.git (see discussion above), then I suspect you need to:
  • add tmp/pot/volume.pot and tmp/pot/veracrypt-mounter-ui.pot to POTFILES.in (I think that’s what create_pot will generate)
  • drop the bits you added to po/POTFILES.skip
  • and by the way, I don’t see the need to use two different translation domains (veracrypt-mounter-ui and veracrypt-mounter): usually one per program is enough

#24 Updated by segfault 2018-07-04 18:57:56

• Assignee changed from segfault to intrigeri
• QA Check set to Ready for QA

intrigeri wrote:
> I had a first quick look as part of Feature #14481. I did not review the VeraCrypt Mounter codebase itself yet and I did not test it. So I’m only commenting about the plumbing:
>
> * It’s a bit annoying to drop stuff directly into config/chroot_local-includes/usr/local/lib/python3.5/dist-packages:

Fully agreed.

> for example, that won’t work on Buster, so once this is merged into devel and then into feature/buster, it’ll be a bit painful to maintain two copies living in different places.

Yeah, I know that. That’s why at first I put it in /usr/local/lib/python3, but turns out that this is not part of Python’s search paths. And I figured that it wouldn’t be too painful to change the path in the Buster branch, but we could also use /usr/lib/python3 instead.

> VeraCrypt Mounter is a big enough project to warrant its own Git repo.

Which you can find here: https://gitlab.com/segfault3/veracrypt-mounter.

> Maybe it’s not worth going through the whole “custom Debian package” thing at least for now,

Yes, I know a Debian package would be the best, but I didn’t have the time for now.

> so I would use a Git submodule which seems like a good trade-off (not too much overhead, but still way cleaner). But then we’ll need to add this new repo to the Transifex setup and integrate this into the release process, so that’s perhaps too much overhead still.

I didn’t think about that.

> So well, maybe find a way to track this in tails.git but without hard-coding OS release -specific Python version in the path?

Do you think it would be ok to put it in /usr/lib/python3?

> * commit:9523245b705564e3d95832642711ce4dc617cf2b feels weird: I doubt config/chroot_local-includes/usr/share/veracrypt-mounter/ui/*.ui belong to a list of POT files;

I indeed forgot to add tmp/pot/veracrypt-mounter-ui.pot and remove the Glade files from POTFILES.in. But note that the result is the same. I won’t pretend that I really understand any of this, but the files listed in POTFILES.in are passed to intltool-merge in refresh-translations, which can apparently parse POT files, Glade files, and desktop.in (which are also listed in POTFILES.in). So if the Glade files are passed to intltool-merge directly the result is the same as passing the POT file created from the Glade files.

> if this is going to stay in tails.git (see discussion above), then I suspect you need to:
> add tmp/pot/volume.pot and tmp/pot/veracrypt-mounter-ui.pot to POTFILES.in (I think that’s what create_pot will generate)
> drop the bits you added to po/POTFILES.skip

I don’t understand why, but as I wrote in the commit messages, intltool complains if I remove the bits from POTFILES.skip.

> and by the way, I don’t see the need to use two different translation domains (veracrypt-mounter-ui and veracrypt-mounter): usually one per program is enough

If we use the same domain, xgettext gets called twice (once for Python files and once Glade files) with the same output file, so the second call will overwrite the result of the first.

#25 Updated by intrigeri 2018-07-05 07:35:36

• Description updated

#26 Updated by intrigeri 2018-07-05 09:41:07

• Assignee changed from intrigeri to segfault
• QA Check changed from Ready for QA to Dev Needed

> Yeah, I know that. That’s why at first I put it in /usr/local/lib/python3, but turns out that this is not part of Python’s search paths. And I figured that it wouldn’t be too painful to change the path in the Buster branch, but we could also use /usr/lib/python3 instead.

I think that Git is pretty good at applying diffs to the renamed version of a file when merging so maybe doing git mv on the Buster branch would work smoothly enough. One will need to pass the right --find-renames= option to git diff when reviewing devel..feature/buster though, which adds some complexity, so let’s try to find a cheap way to avoid it.

Would it work to add PYTHONPATH=/usr/local/lib/python3 in /etc/environment? That’s what we do for Node.js’ NODE_PATH.

>> VeraCrypt Mounter is a big enough project to warrant its own Git repo.

> Which you can find here: https://gitlab.com/segfault3/veracrypt-mounter.

:) → added to the ticket description.

>> * commit:9523245b705564e3d95832642711ce4dc617cf2b feels weird: I doubt config/chroot_local-includes/usr/share/veracrypt-mounter/ui/*.ui belong to a list of POT files;

> I indeed forgot to add tmp/pot/veracrypt-mounter-ui.pot and remove the Glade files from POTFILES.in.

I see you’ve now done it and it LGTM so I’ve merged your branch into the one in the official tails.git (that has current devel merged into it).

> But note that the result is the same. I won’t pretend that I really understand any of this, but the files listed in POTFILES.in are passed to intltool-merge in refresh-translations, which can apparently parse POT files, Glade files, and desktop.in (which are also listed in POTFILES.in). So if the Glade files are passed to intltool-merge directly the result is the same as passing the POT file created from the Glade files.

That’s good news. I’ve taken advantage of this, see below.

>> if this is going to stay in tails.git (see discussion above), then I suspect you need to:
>> add tmp/pot/volume.pot and tmp/pot/veracrypt-mounter-ui.pot to POTFILES.in (I think that’s what create_pot will generate)
>> drop the bits you added to po/POTFILES.skip

> I don’t understand why, but as I wrote in the commit messages, intltool complains if I remove the bits from POTFILES.skip.

OK. I’ve found a workaround that I find slightly less dirty, see 6bf9f8bcae62edc0d261bfd4c6bed1fe7e987303 (adding a comment to the code might be relevant). I think you’ve found a bug in intltool-update --maintain. I can reproduce it with a minimal Python file that only imports gettext and sets a variable to _("test string"). Would be nice to report it if it’s not tracked upstream yet.

>> and by the way, I don’t see the need to use two different translation domains (veracrypt-mounter-ui and veracrypt-mounter): usually one per program is enough

> If we use the same domain, xgettext gets called twice (once for Python files and once Glade files) with the same output file, so the second call will overwrite the result of the first.

Sorry the whole thing is complex and can be very confusing. I got confused myself above due to the misleading “domain” terminology in prog_potfile. For now, please assume s/domain/potfile_basename/ in that function.

I’ve fixed the integration and pushed to the topic branch in the official tails.git, you’ll want to port this to the dedicated Git repo for the app. But that’s not enough due to the app using the wrong translation domain. See below for details.

• It’s actually even worse than what you’re describing above: xgettext will get called once per Python file and once per Glade file. So for example, tmp/pot/veracrypt-mounter-ui.pot will first be generated with strings from /usr/share/veracrypt-mounter/ui/main.ui and then overwritten with strings from /usr/share/veracrypt-mounter/ui/volume.ui. This kind of problems will happen as soon as prog_potfile returns the same value for different inputs we pass to it, which is the case given the globs you added in that function for /usr/share/veracrypt-mounter/ui/* and /usr/local/lib/*/veracrypt_mounter/*.
• Ultimately, all strings managed by refresh-translations will end up in tails.pot and thus tails.mo i.e. the “tails” domain; the other POT files are merely artifacts of intermediate build steps. Every program whose translations are managed this way must use the “tails” domain: git grep -w -E 'textdomain|TEXTDOMAIN|gettext\.install|gettext\.domain'. So:
  • If we’re going to keep managing translations for VeraCrypt Mounter in tails.git, then we need to use the “tails” domain everywhere in VeraCrypt Mounter. But then VeraCrypt Mounter won’t find its translations outside of a system that actually has translations for the “tails” gettext domain. I guess this depends on whether we’re going to keep bundling a copy of that code + Glade files directly in tails.git.
  • In passing, I don’t see how Gettext.bindtextdomain('gnome-shell-extension-status-menu-helper', localeDir) in config/chroot_local-includes/usr/share/gnome-shell/extensions/status-menu-helpertails.boum.org/lib.js@ can possibly work. I think it’s useless and strings get translated only thanks to const Gettext = imports.gettext.domain('tails') (in extension.js).

Also:

• veracrypt-mounter.desktop is not translatable. See how we handle other *.desktop.in.
• placeholder_label is not translatable. Once it’s fixed, add tmp/pot/veracrypt-mounter-volume_manager.py.pot to POTFILES.in.
• The way volume’s “Short description for display to the user” is handled in not i18n’d. Once that’s fixed add tmp/pot/veracrypt-mounter-volume.py.pot to POTFILES.in.

#27 Updated by segfault 2018-07-05 21:30:50

• Assignee changed from segfault to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> Would it work to add PYTHONPATH=/usr/local/lib/python3 in /etc/environment? That’s what we do for Node.js’ NODE_PATH.

Yes, I guess that would work too. Implemented in 718a64cb66158d117a7d564ee460f15528507f92.

> OK. I’ve found a workaround that I find slightly less dirty, see 6bf9f8bcae62edc0d261bfd4c6bed1fe7e987303 (adding a comment to the code might be relevant).

Ugh. I find that very ugly. It’s ok in volume-list.py, but we would have to do the same in multiple places in volume.py (see below), and that would really decreases readability and code style. If this is a bug in how intltool handles Python files, why not use POTFILES.skip? The only purpose of this file seems to be to make intltool ignore certain files. Seems like the right workaround to me. For now I reverted your commit in my branch.

> I think you’ve found a bug in intltool-update --maintain. I can reproduce it with a minimal Python file that only imports gettext and sets a variable to _("test string"). Would be nice to report it if it’s not tracked upstream yet.

I’m hesitant to create a bug about this myself because (1) I don’t feel like I really understand how intltool works, and (2), IIUC, what we found is that intltool-update --maintain seems to be completely broken with all .py files. I find it unlikely that this was never noticed before, and more likely that I’m not using it correctly.

> I’ve fixed the integration and pushed to the topic branch in the official tails.git, you’ll want to port this to the dedicated Git repo for the app. But that’s not enough due to the app using the wrong translation domain. See below for details.

Thanks. But I guess you didn’t mean to remove tmp/pot/veracrypt-mounter.pot from POTFILES.in, because now the translations of /usr/local/bin/veracrypt-mounter are missing. I added it again.

> * It’s actually even worse than what you’re describing above: xgettext will get called once per Python file and once per Glade file. So for example, tmp/pot/veracrypt-mounter-ui.pot will first be generated with strings from /usr/share/veracrypt-mounter/ui/main.ui and then overwritten with strings from /usr/share/veracrypt-mounter/ui/volume.ui. This kind of problems will happen as soon as prog_potfile returns the same value for different inputs we pass to it, which is the case given the globs you added in that function for /usr/share/veracrypt-mounter/ui/* and /usr/local/lib/*/veracrypt_mounter/*.
> * Ultimately, all strings managed by refresh-translations will end up in tails.pot and thus tails.mo i.e. the “tails” domain; the other POT files are merely artifacts of intermediate build steps. Every program whose translations are managed this way must use the “tails” domain: git grep -w -E 'textdomain|TEXTDOMAIN|gettext\.install|gettext\.domain'. So:
> If we’re going to keep managing translations for VeraCrypt Mounter in tails.git, then we need to use the “tails” domain everywhere in VeraCrypt Mounter. But then VeraCrypt Mounter won’t find its translations outside of a system that actually has translations for the “tails” gettext domain. I guess this depends on whether we’re going to keep bundling a copy of that code + Glade files directly in tails.git.

I changed to domain to “tails” for now.

> In passing, I don’t see how Gettext.bindtextdomain('gnome-shell-extension-status-menu-helper', localeDir) in config/chroot_local-includes/usr/share/gnome-shell/extensions/status-menu-helpertails.boum.org/lib.js@ can possibly work. I think it’s useless and strings get translated only thanks to const Gettext = imports.gettext.domain('tails') (in extension.js).

I think you are right. But just for once this is not my fault ;) It’s already been there in e55f05d80710993c4c6afe11359176774668b87d.

> * veracrypt-mounter.desktop is not translatable. See how we handle other *.desktop.in.

Done.

> * placeholder_label is not translatable. Once it’s fixed, add tmp/pot/veracrypt-mounter-volume_manager.py.pot to POTFILES.in.

Oops, actually I removed volume_manager.py during my refactoring, but apparently forgot to remove it from the Tails branch too. I fixed that now.

> * The way volume’s “Short description for display to the user” is handled in not i18n’d. Once that’s fixed add tmp/pot/veracrypt-mounter-volume.py.pot to POTFILES.in.

Indeed, there were also some other strings in volume.py which were not translated.

#28 Updated by intrigeri 2018-07-06 12:02:22

• Assignee changed from intrigeri to segfault
• QA Check changed from Ready for QA to Dev Needed

> intrigeri wrote:
>> Would it work to add PYTHONPATH=/usr/local/lib/python3 in /etc/environment? That’s what we do for Node.js’ NODE_PATH.

> Yes, I guess that would work too. Implemented in 718a64cb66158d117a7d564ee460f15528507f92.

LGTM.

>> OK. I’ve found a workaround that I find slightly less dirty, see 6bf9f8bcae62edc0d261bfd4c6bed1fe7e987303 (adding a comment to the code might be relevant).

> Ugh. I find that very ugly. It’s ok in volume-list.py, but we would have to do the same in multiple places in volume.py (see below), and that would really decreases readability and code style. If this is a bug in how intltool handles Python files, why not use POTFILES.skip? The only purpose of this file seems to be to make intltool ignore certain files. Seems like the right workaround to me. For now I reverted your commit in my branch.

Fair enough.

>> I think you’ve found a bug in intltool-update --maintain. I can reproduce it with a minimal Python file that only imports gettext and sets a variable to _("test string"). Would be nice to report it if it’s not tracked upstream yet.

> I’m hesitant to create a bug about this myself because (1) I don’t feel like I really understand how intltool works, and (2), IIUC, what we found is that intltool-update --maintain seems to be completely broken with all .py files. I find it unlikely that this was never noticed before, and more likely that I’m not using it correctly.

I understand. Actually, I think that intltool-update behaves exactly as documented and the workaround I had put in place simply made it not notice that the Python file had translatable strings. So adding the file to POTFILES.skip seems correct.

>> I’ve fixed the integration and pushed to the topic branch in the official tails.git, you’ll want to port this to the dedicated Git repo for the app. But that’s not enough due to the app using the wrong translation domain. See below for details.

> Thanks. But I guess you didn’t mean to remove tmp/pot/veracrypt-mounter.pot from POTFILES.in, because now the translations of /usr/local/bin/veracrypt-mounter are missing. I added it again.

Indeed, sorry.

>> * The way volume’s “Short description for display to the user” is handled in not i18n’d. Once that’s fixed add tmp/pot/veracrypt-mounter-volume.py.pot to POTFILES.in.

> Indeed, there were also some other strings in volume.py which were not translated.

I believe this:

return _("{volume_size} Volume".format(volume_size=self.size_for_display))

is not correct. I think you want this instead:

return _("{volume_size} Volume").format(volume_size=self.size_for_display)

Generating string by appending stuff in the order that works in English (e.g. desc += _(" (Read-Only)")) is not i18n-friendly: depending on the language, things can be in some order (or direction) or the other. You need one single call to _ per string and use placeholders to replace the bits you want.

#31 Updated by segfault 2018-07-06 17:49:49

• Assignee changed from segfault to intrigeri
• QA Check changed from Dev Needed to Ready for QA

#34 Updated by intrigeri 2018-07-07 10:03:16

• Assignee changed from intrigeri to segfault
• QA Check changed from Ready for QA to Dev Needed

segfault wrote:
> intrigeri wrote:
> > is not correct. I think you want this instead:
> > […]
> Right, fixed.

Good!

> > Generating string by appending stuff in the order that works in English (e.g. desc += _(" (Read-Only)")) is not i18n-friendly: depending on the language, things can be in some order (or direction) or the other. You need one single call to _ per string and use placeholders to replace the bits you want.
>
> I see. I tried to do that with all strings for which it seems to make sense to me.

OK. So the only remaining ones are " – " + self.backing_file_name and " – " + self.drive_name. I’m not quite sure what this is about (random guess: did you mean desc + " – " + self.drive_name? which would need to be converted to .format as well) but that’s not a problem. Anyway: please test and make sure that this works fine in a RTL locale.

> > Also, please add the new files you’re generating to .gitignore
>
> Done.

Tested, confirmed it works: no untracked files left in git status after refreshing translations.

#35 Updated by segfault 2018-07-08 11:20:11

• Assignee changed from segfault to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> segfault wrote:
> > intrigeri wrote:
> > > is not correct. I think you want this instead:
> > > […]
> > Right, fixed.
>
> Good!
>
> > > Generating string by appending stuff in the order that works in English (e.g. desc += _(" (Read-Only)")) is not i18n-friendly: depending on the language, things can be in some order (or direction) or the other. You need one single call to _ per string and use placeholders to replace the bits you want.
> >
> > I see. I tried to do that with all strings for which it seems to make sense to me.
>
> OK. So the only remaining ones are " – " + self.backing_file_name and " – " + self.drive_name. I’m not quite sure what this is about (random guess: did you mean desc + " – " + self.drive_name?

Indeed, desc got lost in my most recent commit. Fixed now.

> which would need to be converted to .format as well)

Done.

> Anyway: please test and make sure that this works fine in a RTL locale.

I would first need translations in a RTL locale, no? I don’t know how to add those. And I don’t see what exactly could not work fine in a RTL locale, given that I now let the translators handle all the strings of the description.

#39 Updated by intrigeri 2018-07-09 11:18:56

• Assignee changed from intrigeri to segfault
• QA Check deleted (Ready for QA)

• I expected to find the launcher next to Disks, in the Utilities part of the apps menu.
• VeraCrypt Mounter lets me Add the same file container twice (but then their state is not synchronized so it’s confusing). Maybe this should not be allowed?
• Clicking Open a second time after unlocking a file container, if Files is still open in the mountpoint, shows the hourglass cursor for a few seconds, but I don’t know what I am waiting for since the operation I requested was already completed (the Files window was raised and got the focus).
• I could not open a file container with right-click (but that’s probably off-topic on this ticket).

I don’t know which ones are blockers for 3.9 but in any case I think none of these should delay the beta any further.

Other than that, I’ve tried to trigger bugs by testing all kinds of corner cases I could and I failed, so the thing feels pretty robust! :)

#40 Updated by segfault 2018-07-10 21:10:45

• Assignee changed from segfault to intrigeri
• QA Check set to Ready for QA

intrigeri wrote:
> * I expected to find the launcher next to Disks, in the Utilities part of the apps menu.
> * VeraCrypt Mounter lets me Add the same file container twice (but then their state is not synchronized so it’s confusing). Maybe this should not be allowed?

Mmh, indeed, that could be a problem. I verified that if you unlock the same container twice, and modify the content in both mounts, the changes to one of them are lost (when the last container is locked, it overwrites the changes of the first). Note that the same happens if you use Disks to attach multiple disk images with read-write access - and that VeraCrypt Mounter also displays the devices added via Disks (or any other tool). But we could at least prevent that the same container is added twice via VeraCrypt Mounter.

I implemented something in 781acdc88072f42a52f9916c8ababa1eeff91e6c.

> * Clicking Open a second time after unlocking a file container, if Files is still open in the mountpoint, shows the hourglass cursor for a few seconds, but I don’t know what I am waiting for since the operation I requested was already completed (the Files window was raised and got the focus).

I was able to reproduce this. I fixed it in 51bcae2e33367de8b18a2d00a5b125a763d0a69d (although it’s a bit sad, because I was using the recommended way (gtk_show_uri_on_window) and now I resorted to subprocess + xdg-open).

> * I could not open a file container with right-click (but that’s probably off-topic on this ticket).

Did the file container have the .tc/.hc extension? If not, it’s expected that you don’t find an entry for mounting in the context menu.

> Other than that, I’ve tried to trigger bugs by testing all kinds of corner cases I could and I failed, so the thing feels pretty robust! :)

Cool!

#46 Updated by segfault 2018-07-19 00:49:37

• Assignee changed from intrigeri to segfault
• QA Check deleted (Ready for QA)

sajolida wrote:
> I finally took the time to try the beta and VeraCrypt Mounter. It’s amazing that you put this out so fast and the result is great!!!

Thanks! :)

> Some comments regarding the UI. Nothing super critical but I think we can polish and simplify things a bit…
>
> * I’m using “partition” in the doc and you’re using “device” in VeraCrypt Mounter. I want to use a consistent terminology between the UI and the doc.
> I checked what VeraCrypt does upstream and it’s a mess: VeraCrypt Format uses “partition/drive”, VeraCrypt uses “device” in the main windows and the Volumes menu and “partition/drive” in the System menu.
> During the user testing, “partition” in the doc was confusing to P4 because she was trying to unlock the whole disk and not only a partition, so we shouldn’t use “partition” only.
> I don’t see any advantage to “device” over “partition or drive” which is more specific and used in more places in the native interface. Do you? Shall we do that?

I’m fine with “Partitions and drives”, I think I only chose “Devices” because it’s shorter.

> * Several of your tooltips provide no additional information on top of the label of the button. For example “[Unlock] (Unlock this volume)” “[Lock] (Lock this volume)”. I think we should remove them. The one on [Open] is good.

Ok

> * I thought that you would automated triggering the unlock dialog and opening Files.
> After specifying which file container they want to open, why do we ask the user to do more work (understand the “Unlock” and “Open” buttons and click on them)? They already said that they want to open this container, no?
> I think that’s why in our original wireframe instead of an [Add] button we had a [Browse…] button. Users are not adding a file container to a list but they are selecting a file container that we want to open and use.
> I like the list of open containers as a way of inspecting which containers are already unlocked but I don’t think we should require more work from the user when opening one.
> Put differently, is it useful in any way to have a file container in state that is attached but not unlocked? I understand that this is useful when unlocking the container fails as we don’t want to ask people to browser for the same container again. In such a case indeed, it’s good to have an [Unlock] button in the list but not otherwise.

One thing I want to clarify is that VeraCrypt Mounter doesn’t only list the volumes added via VeraCrypt Mounter, but all the TCRYPT volumes listed by the GVfs volume monitor. This includes volumes added via Disks, command line or whatever. So it does make sense to show the [Unlock] button for volumes which are not unlocked yet. That being said, I agree that users who add a volume via VeraCrypt Mounter most probably want to unlock it, so it does make sense to trigger the unlock operation for those automatically.

> * Why not replace both the [Detach] and [Lock] button with a [x] button like we have in “Additional Software” (a standard GNOME pattern)? See https://git.tails.boum.org/ux/plain/additional%20software/png/additional%20software.png.
> In the end these two buttons do the same thing: close the file container and remove it from the list.

Actually, whether locking the volume also detaches it depends on how the volume was attached, more specifically, whether the Autoclear property is set (which it is by default when using VeraCrypt Mounter / udisks). But we could also go the extra mile and make sure that even if the Autoclear property was not set, the loop device is still detached when clicking the [X] button.

> * If you agree with my last two comments, we could be left with three possible buttons inside the list:
> [Unlock] after the container is added in case unlocking failed (eg. wrong password).
> [Open] after the container is unlocked. Even if we open Files automatically, I think it makes sense to keep it there.
> [X] to replace both [Detach] and [Lock].

I agree.

> I’m leaving this assigned to intrigeri for his review.

I’m reassigning it to myself to implement the above changes.

#51 Updated by segfault 2018-07-29 14:28:31

• File Buttons with icons.png added
• File Buttons with labels.png added
• Assignee changed from segfault to sajolida
• QA Check set to Info Needed

I implemented the discussed changes.

Now that we’re using an icon for the [Detach] and [Lock] buttons, I think it would look better if we also used icons for the [Unlock] and [Open] buttons. I’m attaching two screenshots, for comparison.

I’m not sure if I find it an improvement that the volume is opened in Nautilus automatically after unlocking, because:

• this happens after some seconds of delay, because the unlocking takes time. The user might start doing other stuff, so that the Nautilus window will steal their focus.
• Mounting itself already shows a notification, which opens the mount point in Nautilus when clicked on. Opening the Nautilus window automatically makes this notification useless noise, but we can’t disable it.

#56 Updated by intrigeri 2018-08-07 22:34:56

• Assignee changed from sajolida to intrigeri
• QA Check changed from Info Needed to Ready for QA

> It’s definitely not a blocker, it would be only a minor change. You can start reviewing.

OK, assigning to me then! Please file another ticket for that minor change so it does not get in the way of my review.

#60 Updated by intrigeri 2018-08-09 10:17:43

• Assignee changed from intrigeri to sajolida

Great job! Code review passes at commit with no red flags for merging on the VeraCrypt Mounter side (except the icon subtask).

Nitpicking:

• I’d rather the warning variable be a dict instead of an array.
• I’m not sure all logger.info should really be at that level. Perhaps some should become logger.debug now that we’re close to production state?

I’ll now express on Redmine, somehow, what’s blocking us from merging the whole branch into devel.

Sending this back to sajolida’s plate since it seems his input was requested on a pending discussion above.

#61 Updated by sajolida 2018-08-10 21:09:42

• File cross.png added
• Assignee changed from sajolida to segfault

Hey! I’m sorry for not answering you about the icons earlier. I totally missed that!

As a general rule, I’m really not a fan of icons without labels. I know it’s a common practice in GNOME for example, and more and more on the web. But I’m also not the only one concerned about that:

https://www.nngroup.com/videos/icon-text-labels/

As an example, during the user testing P4 said she saw the “Unlock” icon in Disks but « didn’t go anywhere near that because she thought it meant it was unlocked already ».

In this case I thought that you would not use a “3D” button for the [X] but instead put the cross directly on the white background. See the (bad) screenshot in attachment. I probably wasn’t clear on that and took it for granted because Alan did that for Additional Software.

Then we’re left with the [Open] and [Unlock] buttons. I think I prefer them with labels than icons for the reason mentioned above but since I haven’t answered you in 2 weeks and you seem to prefer them with icon, I’m fine with what you implemented already unless you also share my preferences :)

#63 Updated by segfault 2018-08-12 19:43:18

• File buttons with icons (X button without relief).png added
• File buttons with labels (X button without relief).png added
• QA Check deleted (Ready for QA)

sajolida wrote:
> As a general rule, I’m really not a fan of icons without labels. I know it’s a common practice in GNOME for example, and more and more on the web.

I also think that buttons without labels can often be confusing. Best example for this is GNOME Disks, IMO.

> As an example, during the user testing P4 said she saw the “Unlock” icon in Disks but « didn’t go anywhere near that because she thought it meant it was unlocked already ».

I see. I thought that the two icons I chose were clear enough, but if the button with the unlock icon is perceived as a status icon, then we should probably go back to labels.

> In this case I thought that you would not use a “3D” button for the [X] but instead put the cross directly on the white background. See the (bad) screenshot in attachment. I probably wasn’t clear on that and took it for granted because Alan did that for Additional Software.

I only saw the additional software config GUI today. I now implemented the relief-less X button.

> Then we’re left with the [Open] and [Unlock] buttons. I think I prefer them with labels than icons for the reason mentioned above but since I haven’t answered you in 2 weeks and you seem to prefer them with icon, I’m fine with what you implemented already unless you also share my preferences :)

I implemented both, see the screenshots. I think it looks better with the icons, but the labels are more clear, so I will keep the label unless you change your mind.

#64 Updated by segfault 2018-08-12 20:16:40

• Assignee changed from segfault to intrigeri
• QA Check set to Ready for QA

intrigeri wrote:
> Great job!

Thanks :)

> Nitpicking:
>
> * I’d rather the warning variable be a dict instead of an array.

Done.

> * I’m not sure all logger.info should really be at that level. Perhaps some should become logger.debug now that we’re close to production state?

Huh, which one do you find to verbose? I think when actions are performed by user interaction (unlocking, locking, opening, etc.) or when the list of volumes changes, log messages are reasonable. I actually added some more logger.info now, because there were none for opening and detaching. The only one I found maybe too verbose is the one in update_volume, I changed it to logger.debug.

I now pushed a few more commits, which implement the changes you requested and the ones discussed with sajolida, and fix two minor bugs I found.

#67 Updated by intrigeri 2018-08-13 08:23:27

• Assignee changed from intrigeri to segfault
• QA Check changed from Ready for QA to Info Needed

> I now pushed a few more commits

I can’t see on the topic branch. Forgot to push?

Also, while you’re at it, please ensure you merge feature/14481-TCRYPT-support-beta from the official repo, that has the up-to-date doc merged it.

#69 Updated by segfault 2018-08-13 08:44:22

• Assignee changed from segfault to intrigeri
• QA Check changed from Info Needed to Ready for QA

#70 Updated by intrigeri 2018-08-13 09:34:38

• blocks deleted (Feature #15223: Iteration 2: Support unlocking VeraCrypt file containers in GNOME)

#76 Updated by intrigeri 2018-08-15 18:21:29

• related to Feature #15680: Icon for VeraCrypt Mounter added

#77 Updated by intrigeri 2018-08-15 19:23:27

• Status changed from In Progress to Resolved
• Assignee deleted (intrigeri)
• QA Check changed from Ready for QA to Pass

Merged into devel!