Project

General

Profile

Feature #6151

Bug #10477: Remove the nmh package

Basic Icedove configuration

Added by Tails over 6 years ago. Updated about 4 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
08/02/2015
Due date:
% Done:

100%

Feature Branch:
kytv:feature/5663-return-to-icedove
Type of work:
Code
Starter:
No
Affected tool:
Email Client

Description

See ideas on the blueprint (#5663), Basic configuration & integration section.


Subtasks


Related issues

Blocks Tails - Feature #10332: Write initial Icedove tests Resolved 10/03/2015

Associated revisions

Revision d6045ce7
Added by anonym about 4 years ago

Merge branch 'feature/5663-return-to-icedove' into devel

Fix-committed: #6151, #9498, #10285, #10332

History

#1 Updated by Tails over 6 years ago

  • Parent task set to #5663

#2 Updated by intrigeri over 6 years ago

  • Priority changed from Normal to High

#3 Updated by BitingBird almost 6 years ago

  • Category set to 176
  • Starter set to No

#4 Updated by intrigeri almost 6 years ago

  • Category deleted (176)

Icedove is a mail client, not a browser.

#5 Updated by BitingBird over 5 years ago

  • Subject changed from basic Icedove configuration to Basic Icedove configuration

#6 Updated by sajolida over 5 years ago

  • Priority changed from High to Normal

#7 Updated by intrigeri over 5 years ago

  • Category set to 212

#8 Updated by BitingBird about 5 years ago

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

#10 Updated by intrigeri over 4 years ago

  • Assignee set to kytv
  • Target version set to 246

#12 Updated by kytv over 4 years ago

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

#13 Updated by kytv over 4 years ago

  • 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 (#9882, #10007).

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

#14 Updated by kytv over 4 years ago

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

#15 Updated by kytv over 4 years ago

Merging all of my branches together has passed my manual testing. Before I set this back to RfQA I want to run the test suite to make perfectly sure that I didn't break anything during the merge resolution.

#16 Updated by kytv over 4 years ago

  • 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 over 4 years ago

#18 Updated by anonym over 4 years ago

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

#19 Updated by kytv over 4 years ago

  • QA Check changed from Dev Needed to Ready for QA

#20 Updated by kytv over 4 years ago

  • Assignee changed from kytv to anonym

#21 Updated by intrigeri over 4 years ago

  • 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 #6154, rather than what we decided to do for the 1st stage?

#22 Updated by kytv over 4 years ago

intrigeri wrote:

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 #6154, rather than what we decided to do for the 1st stage?

That's what I expected from pref("vendor.name", "Tails") but it does not currently disable the Torbirdy account wizard. That was part of my confusion with #10009.

Edit: No, I'm mistaken. The changes are very subtle. I'll push a change to comment out that setting.

#23 Updated by kytv over 4 years ago

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

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

#24 Updated by anonym over 4 years ago

  • 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 #6151, #9498, #9882, #10285, #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
<pre>
+    When I open Icedove's Add-ons Manager
+    Then the Add-ons Manager opens
</pre>

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! :)
Or something like that? Note that the 'I have not configured an email account' step then probably have to handle the case where we don't even have a profile.

#25 Updated by anonym over 4 years ago

I discovered an issue: On start (possibly not the first one; I got it after a reboot with an existing, persistent profile) I'm prompted (in the bottom) this: "Would you like to help Icedove Mail/News by automatically reporting memory usage, performance, and responsiveness to Mozilla? Learn More". We obviously want to provide a default no. I believe one of the toolkit.telemetry prefs can be set to prevent that.

#26 Updated by anonym over 4 years ago

Another issues: despite my en_US locale, I get "Spanish (CU)" as the default spell-checking language when composing email. Even stranger: if I look in Preferences -> Composition -> Spelling, "Arabic" is set to the default.

#27 Updated by anonym over 4 years ago

We should set mail.tabs.autoHide to true to get back some screen estate in single-tab mode.

#28 Updated by anonym over 4 years ago

Actually, the GUI is quite bloated... some things to consider to de-bloat it and get more screen estate:
  • Disable the Quick Filter Bar. It has a button in the tool bar for quick activation. Perhaps it is already off by default?
  • Disable the Status Bar. It reports that Torbirdy is enabled (not interesting, but your automated test needs to be updated), total amount of emails in current view/folder (also not very interesting) and the number of unread ones (duplicated, already visible in the folder pane).

#29 Updated by kytv over 4 years ago

anonym wrote:

Another issues: despite my en_US locale, I get "Spanish (CU)" as the default spell-checking language when composing email. Even stranger: if I look in Preferences -> Composition -> Spelling, "Arabic" is set to the default.

O.o

#30 Updated by kytv over 4 years ago

anonym wrote:

Actually, the GUI is quite bloated... some things to consider to de-bloat it and get more screen estate:
  • Disable the Quick Filter Bar. It has a button in the tool bar for quick activation. Perhaps it is already off by default?

This I've not been able to figure out how to do it. It seems to be saved to the session.json file but I've not had luck setting it to off before an account is created. Any ideas?

  • Disable the Status Bar. It reports that Torbirdy is enabled (not interesting, but your automated test needs to be updated), total amount of emails in current view/folder (also not very interesting) and the number of unread ones (duplicated, already visible in the folder pane).

Changed locally (and the test has been updated).

The issues pointed out earlier have been addressed in my local branch. I'll update this with the commit IDs once I can confirm everything is working as it should.

#31 Updated by kytv over 4 years ago

  • 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 #6151, #9498, #9882, #10285, #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 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 e0fe9cd

[...]

Why are we doing this? Could use a comment.

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

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

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

woohoo.

Telemetry disabled in ee70b0c. Spell checking fixed by adding the branding extension in 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 over 4 years ago

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

#33 Updated by anonym over 4 years ago

kytv wrote:

anonym wrote:

Actually, the GUI is quite bloated... some things to consider to de-bloat it and get more screen estate:
  • Disable the Quick Filter Bar. It has a button in the tool bar for quick activation. Perhaps it is already off by default?

This I've not been able to figure out how to do it. It seems to be saved to the session.json file but I've not had luck setting it to off before an account is created. Any ideas?

I had a quick look and couldn't figure it out either. Perhaps you can have a look if the quick filter button has some interesting "default_visibility" value (so we could fix this with a userChrome.css hack)? That's my last idea. Let's forget about this if that doesn't work out.

#34 Updated by kytv over 4 years ago

anonym wrote:

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.

Since I didn't have an existing clone of the persistence-setup repo I created a new account/repo at gitlab.

This preset has been added to https://gitlab.com/kytv/persistence-setup.git feature/9499-persistence-preset-for-icedove

#35 Updated by sajolida over 4 years ago

Actually, the GUI is quite bloated...

In my opinion, the GUI being bloated or not is not our business but
Thunderbird's and we should differ from the default only if it makes a
particular sense in the context of Tails.

  • Disable the Quick Filter Bar. It has a button in the tool bar for quick activation. Perhaps it is already off by default?

There are two search tools in the Thunderbird toolbar:

  • The "normal" search which is always visible in the toolbar (on the
    same level than the "Get Messages" button). This one searches in the
    body and headers of emails and is pretty much useless when you have a
    good number of encrypted emails. At least I don't like it but that's the
    way it is :)
  • The "quick" search which filters the emails displayed in the right
    pane. I use this one all the time though it's not enabled (unfolded)
    by default. Still, this default saves a bit of vertical space in the
    right pane. I'm not personally happy with it but I don't think we should
    diverge.
  • Disable the Status Bar. It reports that Torbirdy is enabled (not interesting, but your automated test needs to be updated), total amount of emails in current view/folder (also not very interesting) and the number of unread ones (duplicated, already visible in the folder pane).

I don't think we should hide the status bar for the reasons given above.
In this case for example, the status bar is also the place where you see
the progress of downloading email, I use it all the time and especially
in the context of Tails where network can be sloppy, broken, etc.

#36 Updated by intrigeri over 4 years ago

Actually, the GUI is quite bloated... some things to consider to de-bloat it and get more screen estate:

I'm personally reluctant to see us start building our own non-standard Icedove UI on such vague grounds. My concern is that we're going to introduce without noticing bigger problems (e.g. UX ones) than the "quite bloated" one that's justifying the changes. Now, I have no time/energy to put into debating this any further, nor into proving one side or the other of the argument with more solid data, so: whatever :)

#37 Updated by kytv over 4 years ago

Since the majority are opposed to the status bar hiding (I don't feel strongly about it either way), I reverted that change.

Since #6094 has been merged (♥) I updated the tests accordingly. Still to be done: create the upstream tickets.

#38 Updated by anonym over 4 years ago

kytv wrote:

Since the majority are opposed to the status bar hiding (I don't feel strongly about it either way), I reverted that change.

Sure. While I don't feel strongly about this change, I don't at all agree with sajolida's and intrigeri's reasoning. In this particular intance, the status bar is only bloat, and doesn't add anything interesting. That's automatically worse UX, IMHO. Any way...

Since #6094 has been merged (♥) I updated the tests accordingly. Still to be done: create the upstream tickets.

Time is short, so if your remote's branch has new stuff to be reviewed (which it looks like, but I'm not sure if you plan rewriting the history) please let me know ASAP!

Actually, in the future, when the stuff you are blocking on are unrelated to reviewing, please ask for early reviews, making it clear what the unrelated blockers are.

#39 Updated by anonym over 4 years ago

I've merged kytv/feature/5663-return-to-icedove into experimental to give the automated tests some robustness benchmarking.

#40 Updated by kytv about 4 years ago

  • QA Check changed from Dev Needed to Ready for QA

#41 Updated by kytv about 4 years ago

  • Assignee changed from kytv to anonym

#42 Updated by kytv about 4 years ago

anonym wrote:

Is there a ticket for making this configurable in upstream?

[...]

Same for this.

There is now.

https://trac.torproject.org/projects/tor/ticket/17426
https://trac.torproject.org/projects/tor/ticket/17427

#43 Updated by anonym about 4 years ago

  • Status changed from In Progress to 11
  • % Done changed from 0 to 100

#44 Updated by anonym about 4 years ago

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

#45 Updated by anonym about 4 years ago

  • Status changed from 11 to Resolved

Also available in: Atom PDF