Feature #15043

Iteration 4: Create VeraCrypt Mounter application

Added by segfault 2017-12-12 09:20:45 . Updated 2018-08-15 19:23:27 .

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Target version:
Start date:
2018-08-14
Due date:
% Done:

100%

Feature Branch:
segfault:feature/14481-TCRYPT-support-beta
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:
299

Description

During the UX sprint we decided that it would improve the UX if we created an extra application to let the user choose and unlock file containers.
This is not a must, as there will be other ways to unlock file containers. Therefore this will only be done as part of the SponsorW work if we still have time for it after the other work is done.

This application would only exist in Tails.

The canonical location for the source code is currently https://gitlab.com/segfault3/veracrypt-mounter. See below for discussions about how it’s then imported/integrated into Tails.


Files


Subtasks


Related issues

Related to Tails - Feature #15628: Consider re-enabling automounting to improve VeraCrypt UX Resolved 2018-05-30
Related to Tails - Feature #15680: Icon for VeraCrypt Mounter Resolved 2018-06-20
Blocks Tails - Feature #14476: Write user documentation for VeraCrypt support in Tails Resolved 2017-10-08

History

#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

#6 Updated by segfault 2018-01-25 16:07:38

This could also be a cheaper alternative to Feature #15223, which still allows unlocking file containers, even if not well integrated into GNOME.

#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

#10 Updated by intrigeri 2018-06-05 10:30:13

>> 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…

I was confused: I think I got stuck in a mindset where VeraCrypt Mounter was a fallback plan in case we can’t do Feature #15223. As you wrote earlier here, “This could also be a cheaper alternative to Feature #15223, which still allows unlocking file containers, even if not well integrated into GNOME”. And I guess my brain translated “not well integrated into GNOME” into “worse UX”, which is only partly correct. But while all this is true (VeraCrypt Mounter is our fallback plan), VeraCrypt Mounter is not only that: as you’ve reminded me (thanks!) it’s also a nice UX bonus.

Anyway, I think that dropping the target version was the right move: it’s iteration 4, as the description says, it “will only be done as part of the SponsorW work if we still have time for it after the other work is done” and at this point it’s not clear to me whether we’ll have time to do iteration 3. I want to use target versions to help you prioritize your work, which cannot work when you have dozens of tickets assigned for a single version, which is why for example I’ve postponed to 3.9 everything that can wait, and now the 3.8 milestone should reflect what should be worked on next :)

Sorry for the confusion!

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

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

#12 Updated by sajolida 2018-06-06 16:52:35

Answering here Feature #15628#note-6:

> So I would say, let’s try to still implement VeraCrypt Mounter. Do you think it still makes sense to release the current state as the beta? It could give us some data to decide whether we should prioritize VeraCrypt Mounter (although there is not that much I can do with less priority, I still have to upstream the GNOME components ASAP).

I’m ok to do the user testing with the beta as it is now. I’m still not sure if I’ll do the tests this week-end (9-10) or the next (16-17). I really don’t want to stress you out with more work to do in a rush. But if you’re happy to scratch a prototype of VeraCrypt Mounter next week without stressing out, then I’m happy to wait and do the tests on June 16-17 :) I can only bring us better results: better success rate and the testing of VeraCrypt Mounter itself.

#13 Updated by intrigeri 2018-06-06 19:45:39

> But if you’re happy to scratch a prototype of VeraCrypt Mounter next week without stressing out, then […]

(Without having read Feature #15628#note-6 yet, sorry if that makes my comment below stupid:)

Please don’t start working on this before you’ve submitted upstream the pile of patches you’ve got: VeraCrypt Mounter can be written later, e.g. while waiting for GNOME upstream to review the rest of the stuff :)

#14 Updated by segfault 2018-06-07 22:24:37

intrigeri wrote:
> > But if you’re happy to scratch a prototype of VeraCrypt Mounter next week without stressing out, then […]
>
> (Without having read Feature #15628#note-6 yet, sorry if that makes my comment below stupid:)
>
> Please don’t start working on this before you’ve submitted upstream the pile of patches you’ve got: VeraCrypt Mounter can be written later, e.g. while waiting for GNOME upstream to review the rest of the stuff :)

No worries, I know that submitting the remaining patches upstream is urgent and that’s the next work I will do.

#15 Updated by intrigeri 2018-06-08 05:57:23

> No worries, I know that submitting the remaining patches upstream is urgent and that’s the next work I will do.

:)

#16 Updated by sajolida 2018-06-08 09:36:13

Seeing that, I’m going forward with recruiting participants to do the tests this week-end without VeraCrypt Mounter.

#17 Updated by intrigeri 2018-06-08 16:05:09

> Seeing that, I’m going forward with recruiting participants to do the tests this week-end without VeraCrypt Mounter.

Great, this looks like the best strategy towards completion of this project.

#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

#22 Updated by sajolida 2018-06-18 14:44:01

During the user testing of the VeraCrypt beta, all users who didn’t read the doc first (4/5) fell back on reading the doc to be able to unlock a file container. They all searched for “VeraCrypt” in either the Applications menu or the Open With Other Application menu, so VeraCrypt Mounter who have saved them.

#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.

#29 Updated by intrigeri 2018-07-06 12:03:52

Also, please add the new files you’re generating to .gitignore: config/chroot_local-includes/usr/local/share/mime/packages/veracrypt-mounter.xml and config/chroot_local-includes/usr/share/applications/veracrypt-mounter.desktop.

#30 Updated by segfault 2018-07-06 17:49:34

intrigeri wrote:
> I believe this:
>
> […]
>
> is not correct. I think you want this instead:
>
> […]

Right, fixed.

> 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.

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

Done.

#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

#32 Updated by segfault 2018-07-06 21:13:27

I forgot to push my last commits, I’ve done that now.

#33 Updated by intrigeri 2018-07-07 09:55:02

Side-note: I’ve been using https://gitlab.com/segfault_/tails.git as your remote for ages but apparently that account was taken over yesterday. Anyway, I’ll use https://gitlab.com/segfault3/tails now :)

#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.

#36 Updated by segfault 2018-07-08 11:42:24

I added some comments for translators.

#37 Updated by intrigeri 2018-07-08 16:10:09

>> 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.

Fully agreed.

#38 Updated by intrigeri 2018-07-09 10:09:16

segfault wrote:
> I added some comments for translators.

Very nice!

Code review passes for the desc thing. I’ve pushed the updated branch to the official repo, started a build and triggered a full test suite run (to ensure we don’t introduce regressions outside of VeraCrypt support). Then I’ll quickly test VeraCrypt Mounter manually and if happy, I’ll close this as resolved!

#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!

#41 Updated by intrigeri 2018-07-10 21:20:54

Meta: I won’t be able to look at this more closely in the next 10-15 days so don’t block on this for the beta. If you’re super confident that your last changes can, and should, go into the beta with no review, fine. OTOH they clearly fix non-critical stuff so it could as well wait.

> (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).

Bug report? :)

>> * 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.

Indeed, it had a .mp3 extension.

#42 Updated by segfault 2018-07-10 21:50:21

intrigeri wrote:
> Meta: I won’t be able to look at this more closely in the next 10-15 days so don’t block on this for the beta. If you’re super confident that your last changes can, and should, go into the beta with no review, fine. OTOH they clearly fix non-critical stuff so it could as well wait.

I did not include any of the last changes in the Feature #14481 branch and I don’t think they have to be in the beta.

> > (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).
>
> Bug report? :)

Fine, I created an issue: gitlab.gnome.org/GNOME/gtk/issues/1211

#43 Updated by intrigeri 2018-07-11 08:20:23

> Fine, I created an issue: gitlab.gnome.org/GNOME/gtk/issues/1211

Great, added to https://tails.boum.org/blueprint/GNOME_bugs_that_affect_Tails/ :)

#44 Updated by sajolida 2018-07-18 17:03:21

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!!!

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?
  • 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.
  • 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.
  • 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.
    • It would also save users from having the understand what the uncommon action verb “Detach” mean (while I understand that it relates to the underlying technical operation).
    • From Feature #15578 we learned that people are rightfully cautious when performing what seems like a destructive action but that they trust the software to do the right thing (and might need some reassurance when doing so). Here I think that keeping your tooltips would work fine: “Detach this volume” when the container is attached but locked (eg. wrong password) and “Lock this volume” when the container is attached and unlocked. They would reassure people regarding the scary [x] and inform expect users.
  • 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].
  • Hurray to the spinner!

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

#45 Updated by sajolida 2018-07-18 17:43:07

I drafted the documentation for VeraCrypt Mounter as I would like it to behave in 05433a372e. You might want to have a look if you have a hard time understanding my comments.

#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.

#47 Updated by sajolida 2018-07-26 13:16:11

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

I did that in the doc, see 023d4c9e6b. I’m using “partition or drive” in
the doc but, as you said, in the UI I think that “partitions and drives”
is more correct.

> 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.

Understood! So yes, in case the user attaches a file container as disk
image but doesn’t unlock it, VeraCrypt Mounter should list it with an
[Unlock] button.

> 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.

Yes, let’s do that!

#48 Updated by intrigeri 2018-07-28 10:15:02

segfault: it would help me schedule my work if you could tell me when I can expect to start reviewing VeraCrypt Mounter (after it stops being WIP as I’d rather minimize the amounts of review rounds).

#49 Updated by segfault 2018-07-28 13:00:20

I expect that implementing the last changes takes 1-2 hour. It’s the next thing I will work on, beside the merge requests - those have top priority right now IMO. So if I don’t have to work on those all day on Monday, I guess you can start reviewing on Tuesday. Would that work for you?

#50 Updated by intrigeri 2018-07-29 00:31:52

> So if I don’t have to work on those all day on Monday, I guess you can start reviewing on Tuesday. Would that work for you?

Perfect, thanks for the update!

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

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.

#52 Updated by sajolida 2018-08-01 14:04:53

> * 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 takes a few seconds and you implemented some feedback for that.
I guess that in the vast majority of cases people will be waiting for
their unlocking action to finish before moving to something else, no? I
mean, unlocking is not so slow that you would go check your mails until
it finishes, right? Like it would be the case for wiping a drive for
example.

> * 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.

The fact that we can’t disable the notification doesn’t make the opening
of Nautilus useless, isn’t? Let’s decide if we want to open Nautilus or
not (and then if we can’t disable the notification, it will be a minor
glitch).

I’m still in favor of opening it automatically but I won’t fight over
it: do as you wish.

#53 Updated by segfault 2018-08-01 18:55:43

sajolida wrote:
> > * 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 takes a few seconds and you implemented some feedback for that.
> I guess that in the vast majority of cases people will be waiting for
> their unlocking action to finish before moving to something else, no? I
> mean, unlocking is not so slow that you would go check your mails until
> it finishes, right? Like it would be the case for wiping a drive for
> example.

It can take up to the time it takes when you enter an incorrect password (it takes much less time with default parameters, but it will take if you chose more uncommon algorithms when creating the volume), which is 15 seconds on my system. That is definitely too much for my short attention span, so I would go check my mails.

> > * 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.
>
> The fact that we can’t disable the notification doesn’t make the opening
> of Nautilus useless, isn’t? Let’s decide if we want to open Nautilus or
> not (and then if we can’t disable the notification, it will be a minor
> glitch).

Well, if users know that they can open the volume in Nautilus by clicking on the notification, then I guess opening it automatically is pretty useless (maybe they don’t even want to open the volume in Nautilus, but use some other application that accesses the files on the volume). But I don’t know how many users know about this feature - the notification only shows the “Open with Files” label when hovering over it with the mouse.

Note that this is also something that clearly deviates from the way GNOME handles things. When you plug in a USB drive or use Disks to mount a volume, it does not automatically open the volume in Nautilus, but displays the notification to notify the user and let them choose what to do next.

I would prefer it if we would do it the way GNOME usually does, but I don’t have a strong opinion. If after my above arguments you still favor opening it automatically, then I’ll just leave it like this. (This ticket is ready for review then)

#54 Updated by intrigeri 2018-08-07 05:56:03

Any update wrt. when I can start the review? Is the ongoing discussion a blocker?

#55 Updated by segfault 2018-08-07 20:40:11

intrigeri wrote:
> Any update wrt. when I can start the review? Is the ongoing discussion a blocker?

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

#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.

#57 Updated by sajolida 2018-08-08 17:58:26

> Note that this is also something that clearly deviates from the way GNOME handles things. When you plug in a USB drive or use Disks to mount a volume, it does not automatically open the volume in Nautilus, but displays the notification to notify the user and let them choose what to do next.

I’m convinced by the argument of doing like GNOME does :)

#58 Updated by segfault 2018-08-08 20:21:05

sajolida wrote:
> I’m convinced by the argument of doing like GNOME does :)

I think I found a good middle ground: I’m now only opening the volume automatically if it was opened via Nautilus, because then it’s clearly an opening action. If it is unlocked via the “Attach” button in VeraCrypt Mounter, it does not open automatically.

Btw, you didn’t comment on the new icons I added for unlocking and opening. Do you want to keep them?

#59 Updated by segfault 2018-08-08 20:24:39

intrigeri: I think I already wrote that somewhere before: I recommend you review the current state of the code as a whole, instead of going through commit by commit, because I extensively refactored it at times.

#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 :)

#62 Updated by sajolida 2018-08-11 08:16:29

Ah, and for “Open” we could also maybe use a hyperlink like GNOME Disks does. But I’m not sure how it would look in this context.

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

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.

#65 Updated by segfault 2018-08-12 21:00:36

> I think when actions are performed by user interaction (unlocking, locking, opening, etc.) or when the list of volumes changes, log messages are reasonable.

See also udisks, which also prints a log message every time a volume is unlocked, locked, mounted, etc.

#66 Updated by intrigeri 2018-08-13 08:19:44

>> * 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.

OK, great!

> 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.

I’ll review this today.

#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.

#68 Updated by segfault 2018-08-13 08:42:50

intrigeri wrote:
> > I now pushed a few more commits
>
> I can’t see on the topic branch. Forgot to push?

Right, I only pushed it to the VeraCrypt Mounter repository, but forgot to push it to the Tails topic branch.

> 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.

Done.

#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)

#71 Updated by intrigeri 2018-08-13 09:35:25

LGTM. Assuming this is the final version (modulo the remaining subtask) for 3.9, I’ll quickly build & test this once I’ve uploaded the packages (Feature #15521).

#72 Updated by intrigeri 2018-08-13 10:02:23

Deliverable: SponsorW_2017

> 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.

It is a deliverable at least because it was identified as one of the top-priority fixes after user testing (so arguably it would be a subtask of Feature #14480 but let’s not bother).

#73 Updated by intrigeri 2018-08-13 10:57:18

(Sorry for the noise, I’m not 100% used to my new MUA yet so I’ve mistaken the message I was replying to for a new one, while it was 8 months old.)

#74 Updated by sajolida 2018-08-13 16:46:11

Hurray for the labels!

#75 Updated by segfault 2018-08-14 12:23:15

I pushed another small commit to fix the automatic opening after unlocking.

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

#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!