Tails

#1 Updated by Tails 2013-07-18 11:48:23

• Parent task set to Feature #5663

#2 Updated by intrigeri 2013-07-19 02:43:05

• Priority changed from Normal to High

#3 Updated by BitingBird 2014-03-24 04:44:49

• Category set to 176
• Starter set to No

#4 Updated by intrigeri 2014-03-24 08:57:44

• Category deleted (176)

Icedove is a mail client, not a browser.

#5 Updated by BitingBird 2014-06-09 09:55:13

• Subject changed from basic Icedove configuration to Basic Icedove configuration

#6 Updated by sajolida 2014-07-10 20:23:27

• Priority changed from High to Normal

#7 Updated by intrigeri 2014-08-12 13:44:54

• Category set to 212

#8 Updated by BitingBird 2015-01-02 19:32:06

• Blueprint set to https://tails.boum.org/blueprint/Return_of_Icedove__63__

#9 Updated by intrigeri 2015-05-29 12:43:39

• blocks #8668 added

#10 Updated by intrigeri 2015-05-29 12:43:53

• Assignee set to kytv
• Target version set to 246

#11 Updated by intrigeri 2015-08-26 05:59:04

• Deliverable for set to 268

#12 Updated by kytv 2015-10-02 06:16:23

• Status changed from Confirmed to In Progress
• Target version changed from 246 to Tails_1.7

#13 Updated by kytv 2015-10-02 08:56:19

• Assignee changed from kytv to anonym
• QA Check set to Ready for QA
• Feature Branch set to kytv:feature/6151-icedove-config

By necessity (easier merges) this branch includes commits that fix others tickets (Feature #9882, Feature #10007).

It also depends on kytv:feature/10285-add-icedove-to-Tails.

#14 Updated by kytv 2015-10-02 09:31:41

• Assignee changed from anonym to kytv
• QA Check changed from Ready for QA to Dev Needed
• Feature Branch deleted (kytv:feature/6151-icedove-config)

Since there was already a feature/icedove which I’m supposed to use(ugh) I need to redo this branch. Taking it back for a bit.

#16 Updated by kytv 2015-10-03 02:46:56

• Assignee changed from kytv to anonym
• QA Check changed from Dev Needed to Ready for QA
• Feature Branch set to kytv:feature/5663-return-to-icedove

#17 Updated by kytv 2015-10-03 13:33:50

• blocks Feature #10332: Write initial Icedove tests added

#18 Updated by anonym 2015-10-05 01:46:23

• Assignee changed from anonym to kytv
• QA Check changed from Ready for QA to Dev Needed

#19 Updated by kytv 2015-10-05 02:32:32

• QA Check changed from Dev Needed to Ready for QA

#20 Updated by kytv 2015-10-05 02:35:44

• Assignee changed from kytv to anonym

#21 Updated by intrigeri 2015-10-05 05:53:20

• Assignee changed from anonym to kytv
• QA Check changed from Ready for QA to Info Needed

I see pref("vendor.name", "Tails") in your branch. Doesn’t this turn off Torbirdy’s own account setup wizard, and turn on Icedove’s wizard, that is what we want for the second stage, once we have Feature #6154, rather than what we decided to do for the 1st stage?

#23 Updated by kytv 2015-10-05 09:50:41

• Assignee changed from kytv to anonym
• QA Check changed from Info Needed to Ready for QA

I pushed commit:aa23755 to my branch to comment out the vendor.name line. Sorry about that. :S

#24 Updated by anonym 2015-10-07 06:23:45

• Assignee changed from anonym to kytv
• QA Check changed from Ready for QA to Dev Needed

Here’s a review of feature/5663-return-to-icedove (covering tickets Feature #6151, Feature #9498, Feature #9882, Feature #10285, Feature #10332). I’ll review the diff (although I’ve read through the commit messages) since the ooooold history that you rebased your work on just makes things confusing… :)

Buy first: great work! It works perfectly, quite simply put. I only have some nitpicking:

--- /dev/null
+++ b/config/chroot_local-hooks/10-install_icedove-l10n
@@ -0,0 +1,7 @@
+#!/bin/sh
+
+set -e
+
+echo "installing icedove-l10n-all"
+
+apt-get --yes install -t wheezy icedove-l10n-all

Perhaps I’m missing something, but why are we not installing icedove-l10n-all via config/chroot_local-packageslists/tails-common.list?

--- /dev/null
+++ b/config/chroot_local-includes/etc/skel/.icedove/profile.default/preferences/0000tails.js
@@ -0,0 +1 @@
+user_pref("extensions.enigmail.configuredVersion", "1.7.2");

Please update wiki/src/contribute/release_process.mdwn with a reminder to update this pref, similar to the one for AdBlock.

--- /dev/null
+++ b/config/chroot_local-includes/usr/local/bin/icedove
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+set -e
+set -u
+
+ZSH_VERSION="${ZSH_VERSION:-}"
+. gettext.sh
+TEXTDOMAIN="tails"
+export TEXTDOMAIN
+

We are not using gettext in the script, so why adding any localization plumbing at all?

+    TMPDIR="${PROFILE}/tmp"
+    mkdir --mode=0700 -p "$TMPDIR"
+    export TMPDIR

Why are we doing this? Could use a comment.

--- a/config/chroot_local-includes/usr/local/bin/torified-claws-mail
+++ /dev/null

--- a/config/chroot_local-includes/usr/local/sbin/live-persist
+++ b/config/chroot_local-includes/usr/local/sbin/live-persist
@@ -64,6 +64,36 @@ Options affecting the 'activate' action:
 "
 }

+escape_path() {
+       printf "%s\n" $1 | sed 's/\./\\./g'
+}

This only escape dots. And escape for what context? Shell? Ah, regex! :) I suggest you rename it escape_dots(). If you want to make something more generic, like a escape_rege_chars(), you should try something like sed 's/\\([.*?^...]\)/\\\1/g' where you have to figure out the rest of the character class (the ...). That might be painful. Also, extended vs “normal” regex. Bleh. How about escape_dots()? :)

Btw, I really like the migrate_persistence_preset() refactoring! :) Works perfectly, too.

So at the moment the only way to get this preset is to have the Claws Mail preset enabled + migration, but we need to add support to tails-persistence-setup to add it from scratch. If you send me a pull request for a branch adding it I can build a package, upload it to the feature-5663-return-to-icedove APT suite, or similar, so you can add an APT overlay to your branch that installs the new package. Hint: look in lib/Tails/Persistence/Configuration/Presets.pm, and it’s mostly a copy-paste dance.

--- a/config/chroot_local-packageslists/tails-common.list
+++ b/config/chroot_local-packageslists/tails-common.list
@@ -82,6 +82,9 @@ claws-mail-archiver-plugin
 claws-mail-i18n
 claws-mail-pgpinline
 claws-mail-pgpmime
+icedove
+xul-ext-torbirdy
+enigmail
 connect-proxy
 desktop-base
 dmz-cursor-theme

We try to keep the list sorted, which you might want to correct here.

--- /dev/null
+++ b/config/chroot_local-patches/icedove_default_to_IMAP.diff

Is there a ticket for making this configurable in upstream?

--- /dev/null
+++ b/config/chroot_local-patches/torbirdy-adjust-defaults.diff

Same for this.

And now the test suite bits. First the .feature file:

+  Background:

Since each scenario starts with:

+    Given I start "Icedove" via the GNOME "Internet" applications menu
+    And Icedove has started

I think it should be made as part of the Background. Actually, I think it should be:

Background:
  Given a computer
  And I start Tails from DVD and I login
  And I have not configured an email account
  When I start "Icedove" via the GNOME "Internet" applications menu
  And Icedove has started
  Then I am prompted to setup an email account
  And I save the state so the background can be restored next scenario

+ When I open Icedove’s Add-ons Manager
+ Then the Add-ons Manager opens

IMHO the former should invoke the latter via step so we don’t have to explicitly list that step. Or just merge the code? When do we only want to do either of them?

The step definitions look great at first glance! Nothing to nitpick on! :)

#31 Updated by kytv 2015-10-07 16:28:10

• Assignee changed from kytv to anonym
• QA Check changed from Dev Needed to Ready for QA

anonym wrote:
> Here’s a review of feature/5663-return-to-icedove (covering tickets Feature #6151, Feature #9498, Feature #9882, Feature #10285, Feature #10332). I’ll review the diff (although I’ve read through the commit messages) since the ooooold history that you rebased your work on just makes things confusing… :)
>
> Buy first: great work! It works perfectly, quite simply put. I only have some nitpicking:
>
> […]
>
> Perhaps I’m missing something, but why are we not installing icedove-l10n-all via config/chroot_local-packageslists/tails-common.list?

I wondered the same thing tbh. I originally installed it using config/chroot_local-packageslists/tails-common.list but feature/icedove used the hook, so I modified my existing stuff to work around it.

I reverted back to how I did it before that merge in commit:2328fa2
>
> […]
> Please update wiki/src/contribute/release_process.mdwn with a reminder to update this pref, similar to the one for AdBlock.
>

commit:79c4dc3

> […]
>
> We are not using gettext in the script, so why adding any localization plumbing at all?

I thought there might be something added later, though I didn’t know what that might be. Removed in commit:e0fe9cd

> […]
>
> Why are we doing this? Could use a comment.

commit:d60411e

>
> […]
>
> […]
>
> This only escape dots. And escape for what context? Shell? Ah, regex! :) I suggest you rename it escape_dots(). If you want to make something more generic, like a escape_rege_chars(), you should try something like sed 's/\\([.*?^...]\)/\\\1/g' where you have to figure out the rest of the character class (the ...). That might be painful. Also, extended vs “normal” regex. Bleh. How about escape_dots()? :)
>

commit:e8f33d3

> Btw, I really like the migrate_persistence_preset() refactoring! :) Works perfectly, too.

:)
> So at the moment the only way to get this preset is to have the Claws Mail preset enabled + migration, but we need to add support to tails-persistence-setup to add it from scratch. If you send me a pull request for a branch adding it I can build a package, upload it to the feature-5663-return-to-icedove APT suite, or similar, so you can add an APT overlay to your branch that installs the new package. Hint: look in lib/Tails/Persistence/Configuration/Presets.pm, and it’s mostly a copy-paste dance.

Sure.

>
> […]
>
> We try to keep the list sorted, which you might want to correct here.

I’ve done the sorting of the Icedove packages in commit:7c192d6. If sorting more would be of use I can do so.

>
> […]
>
> Is there a ticket for making this configurable in upstream?

Not yet.

>
> […]
>
> Same for this.
>

Not yet.

> And now the test suite bits. First the .feature file:
>
> […]
>
> Since each scenario starts with:
> […]
> I think it should be made as part of the Background. Actually, I think it should be:
>
> […]
>

Changed in commit:fdf768f

> IMHO the former should invoke the latter via step so we don’t have to explicitly list that step. Or just merge the code? When do we only want to do either of them?

I couldn’t think of any cases in which we’d not want both, so merged in commit:7f7d770.
>
> The step definitions look great at first glance! Nothing to nitpick on! :)

woohoo.

Telemetry disabled in commit:ee70b0c. Spell checking fixed by adding the branding extension in commit:9e07c3d. The language under Preferences → Composition → Spelling is still defaulting to Arabic for whatever reason but the spell checking has been in the correct language. (I figured I’d push for a review of these changes while this is looked into)

#32 Updated by anonym 2015-10-08 04:44:26

• Assignee changed from anonym to kytv
• QA Check changed from Ready for QA to Dev Needed

kytv wrote:
> > So at the moment the only way to get this preset is to have the Claws Mail preset enabled + migration, but we need to add support to tails-persistence-setup to add it from scratch. If you send me a pull request for a branch adding it I can build a package, upload it to the feature-5663-return-to-icedove APT suite, or similar, so you can add an APT overlay to your branch that installs the new package. Hint: look in lib/Tails/Persistence/Configuration/Presets.pm, and it’s mostly a copy-paste dance.
>
> Sure.

Note that this is a blocker.

> > Is there a ticket for making this configurable in upstream?
>
> Not yet.

Please create one! Also a blocker. :)

> >
> > […]
> >
> > Same for this.
> >
>
> Not yet.

Please create one! Also a blocker. :)

> > The step definitions look great at first glance! Nothing to nitpick on! :)
>
> woohoo.

Actually I found something: since we hide the menu bar I expect users will use the combined menu button so test using that approach would match what users do more closely. More importantly, from experience, clicking that button should be more robust than relying on keyboard shortcuts, so I think we should do that instead. What do you think?

> The language under Preferences → Composition → Spelling is still defaulting to Arabic for whatever reason but the spell checking has been in the correct language. (I figured I’d push for a review of these changes while this is looked into)

As long as the correct spell checker (vs OS locale) is picked, I don’t care; don’t “waste” (IMHO) time on investigating this! (I only mentioned it as an additional symptom for the spell-checker situation.)

#40 Updated by kytv 2015-10-26 03:06:47

• QA Check changed from Dev Needed to Ready for QA

#41 Updated by kytv 2015-10-26 03:13:36

• Assignee changed from kytv to anonym

#43 Updated by anonym 2015-10-26 13:01:11

• Status changed from In Progress to Fix committed
• % Done changed from 0 to 100

Applied in changeset commit:d6045ce79f7f72d2658ee5e54097ce24be106d73.

#44 Updated by anonym 2015-10-26 13:04:12

• Assignee deleted (anonym)
• QA Check changed from Ready for QA to Pass

#45 Updated by anonym 2015-11-03 11:31:45

• Status changed from Fix committed to Resolved