1637668 - Implement colour customization option for Folder Pane icons

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement, P2)

Description

[Alessandro Castellani [:aleca]]

Implement the option of customizing the colour of the new SVG icons in the Folder Pane.
We can also explore the possibility to implement some defaults to better distinguish the sections and better ease users into the new UI.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: folder-properties.png

Possible implementation for colour customization button in the Folder Properties dialog.

Comment 2

[Artem S. Tashkinov]

Do you seriously believe people with many accounts and folders will go through all of them to set colors? And then you suddenly decide this feature is not worth it and remove it, thus rendering people's grind futile?

Here's a better idea. Since you've basically killed off theming in any shape or form, what about getting an option to install and apply icon theme packs for Mozilla Thunderbird? This way Thunderbird may look better/closer to the underlying OS it's running on.

People who love Gnome/Windows 10 brain-damaged design may use the current icons (bland, gray, indistinguishable crap as of Thunderbird 77), people with a modicum of common sense could use classic colored icons (which are extremely easy to read and overview in an instant), etc. etc. etc. Then the user could apply an icon set for Gnome, KDE, Windows 10, MacOS X, Classic Thundebird, Microsoft Outlook 2003, etc. etc. etc.

Please consider this feature. It shouldn't be too difficult to implement.

Say we have a zip file with inbox.png/sent.png/drafts.png/spam.png/etc. - you get the gist. Or maybe you could also add folders for different resolutions and icons types, e.g. 16x16, 24x24, 32x32, 48x48, SVG, etc.

Comment 3

[Alessandro Castellani [:aleca]]

(In reply to Artem S. Tashkinov from comment #2)

Do you seriously believe people with many accounts and folders will go through all of them to set colors?

Mh, no, we don't assume anything, that's why in the initial comment I wrote:
We can also explore the possibility to implement some defaults to better distinguish the sections and better ease users into the new UI.
Custom colouring is getting introduced to let users control which folder/section they want to highlight.

And then you suddenly decide this feature is not worth it and remove it, thus rendering people's grind futile?

That seems very unlikely, why do you think that?

Here's a better idea. Since you've basically killed off theming in any shape or form

I didn't kill themeing, on the contrary, with this update we reduced code redundancy and optimized the CSS so themes developers can update this whole section with very simple CSS by applying custom icons if they want. How did I kill themeing?

what about getting an option to install and apply icon theme packs for Mozilla Thunderbird? This way Thunderbird may look better/closer to the underlying OS it's running on.

A custom theme could do that.

People who love Gnome/Windows 10 brain-damaged design may use the current icons (bland, gray, indistinguishable crap as of Thunderbird 77)

I love when the work we do is considered "crap".

Please consider this feature. It shouldn't be too difficult to implement.
Say we have a zip file with inbox.png/sent.png/drafts.png/spam.png/etc. - you get the gist. Or maybe you could also add folders for different resolutions and icons types, e.g. 16x16, 24x24, 32x32, 48x48, SVG, etc.

It's not really a matter of how easy/difficult it is to implement, but it's more a matter of maintainability of the code and the resources.
Targeting each platform and each Desktop environment to ship native icons, and have different resolutions for each icons to handle different DPI density is extremely time consuming and not efficent.

Offering a unified, consistent, and easily maintainable UI is our main goal.
Themes can edit those icons and implement custom variations if they want, we're not blocking that.
I understang that these type of UI changes can be jarring for the first couple of days, but I'd suggest you to use it for a couple of days and give it a chance.

Thanks for your feedback.

Comment 4

[Artem S. Tashkinov]

That seems very unlikely, why do you think that?

Full theming support has been removed and an insane amount of work put into it has been wasted.

I didn't kill themeing, on the contrary, with this update we reduced code redundancy and optimized the CSS so themes developers can update this whole section with very simple CSS by applying custom icons if they want. How did I kill themeing

This sounds great however all I can see is light themes which can only touch the header background and nothing else. I don't even understand why they are called "themes". It's a bad joke, not themes.

A custom theme could do that.

Nowhere to be seen. Please write a blog post/make an announcement/write documentation how to theme post-XUL Thunderbird 77.

I love when the work we do is considered "crap".

Sorry for being blunt but the new icons are unreadable:

1. They all look the same
2. They lack contrast
3. They all have the same grayish shade
4. They do not look neat on my standard DPI display which is what's in use by > 98% of your users. I've just tried several times to find a spam folder in one of my accounts and I just don't see it any more without reading folder names one by one from top to bottom, while with "classic" "outdated" icons you could recognize the spam folder icon instantly.

This is what the Gnome desktop environment has been forcing on people for many years already and it's hated by absolute most people. Sorry, that's not "design", that's just crap. Good design is a design which does not require the user to painstakingly discover or work through things - the current icons (Thunderbird 77 beta 2) are a perfect example of that for the reasons listed above.

The human eye takes extra time and effort to process forms and shapes, while colors and contrast are perceived instantly. Unlike shapes colors are remembered and recognized right away.

Please stop. The whole Thunderbird UI has been made a grayish mess recently which is really hard to read vs.

https://addons.thunderbird.net/en-US/thunderbird/addon/ms-office-2003-jb-edition/

See how easy it's on the eyes because you can distinguish everything right away, instantly. Many people refuse to upgrade from Thundebird 60 because you're trying hard to copy Gnome and Windows 10 UI both of which are the worst designs ever created.

Please get colors back. Thank you very much.

Comment 5

[Kai Engert (:KaiE:)]

Artem, I would like to ask that you please read the Mozilla Community Participating guidelines, section "Be Respectful", and reconsider how you're treating others in bugzilla. https://www.mozilla.org/en-US/about/governance/policies/participation/
IMO, describing the work of others in the way you did can be considered an insult.

Comment 6

[Kai Engert (:KaiE:)]

Alessandro, would the scalable image format allow filling the icons with a solid color? For example, could the user folder icons be filled with a classic yellow, the Trash can with green, the Junk fire with orange?

Comment 7

[Kai Engert (:KaiE:)]

Alessandro, is this the right place to make suggestions that are beyond the customization suggestion described in this bug?
Another feedback I have is that the label of folders with new messages changes to blue. However, because the top account icon is blue, too, it's more difficult to distinguish the states "have new message in inbox" and "don't have new message in inbox", because there's always something blue in the upper left.

Comment 8

[Kai Engert (:KaiE:)]

Attachment: test-folders.png

Comment 9

[Artem S. Tashkinov]

This looks good, now can we apply certain colors to folders automatically by default?

Comment 10

[Artem S. Tashkinov]

Attachment: g

For instance I'd like to see the SPAM folder to have a fire-like color by default for all my (actually) five accounts.

User/custom folders could use the yellow color.

Default server folders could be light blue.

In short it would be great to be able to color them all in bulk.

Comment 11

[Alessandro Castellani [:aleca]]

To answer both Kai and Artem, we can definitely consider that.
For now I'm focusing on the first step, which is implementing the colour customization of the current icon stroke, and manage them one by one separately.
Once that's done, we'll have the feature in place to iterate upon.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: filled-folders.png

Comment 13

[Kai Engert (:KaiE:)]

Alessandro, this is just a quick attempt. I cannot judge if this makes sense, that's up to you.
A question is, what is the motivation for the custom stroke colors? I assume the motivation is to make it easier to distinguish the icons. Potentially, if the icons filled with a solid color are considered as sufficient to distinguish the icons, then maybe it could be unnecessary to provide customization for the stroke?

Comment 14

[Kai Engert (:KaiE:)]

Attachment: filled-svg.zip

Comment 15

[Andrei Hajdukewycz [:sancus]]

TBH I think I like the stroke colors better than fill, the filled icons look extra cartoonish to me. I think in terms of visual recognition they're equally useful. But I trust Alessandro to make things look good. :)

I am looking forward to individual color customization as it will enable coloring my many custom folders differently, and leaving the folders I don't care as much about grey, which is pretty cool and very useful.

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: 1637668-folderpane-colours.diff

Comment 17

[Richard Marti (:Paenglab)]

Comment on attachment 9149452 [details] [diff] [review]
1637668-folderpane-colours.diff

Search folders can't be coloured. This should also be possible.

The reset button on Windows is too wide. It needs a min-width: 16px; or something.
On Mac the appearance is a normal button. This needs a -moz-appearance: none; and the icon isn't vertically centred.
Linux looks good. :-)

Comment 18

[Alessandro Castellani [:aleca]]

Attachment: 1637668-folderpane-colours.diff

This implements the color customization also for Virtual folders, but only if the folder already exists, not on creation.

I had to duplicate a little bit of code for the folder properties and virtual folder properties dialogs, which makes me think that if we decide to enable color customization also for the top level folders (email, local folder, rss, etc.) we might consider creating a dedicated dialog to prevent code duplication in 3 different locations.

Comment 19

[Richard Marti (:Paenglab)]

Comment on attachment 9149526 [details] [diff] [review]
1637668-folderpane-colours.diff

Styling looks good. Thanks.

But this patch gives issues:
- I get two times this in the console on start:  Ruleset ignored due to bad selector.      messenger.xhtml:9897:1

A problem with your stylesheet adding?

- In search folder click on "Choose..." or in Account Manager > Synchronisation & Storage click "Advanced" I get
  Uncaught TypeError: can't access property "textContent", this.inlineStyle is null   folderPane.js:2628:5

Both dialogs are missing the dynamic stylesheets. Where could also be other dialogs which use the folderTree.

Comment 20

[Wayne Mery (:wsmwk)]

LMK if you want this uplifted to the next beta

Comment 21

[Alessandro Castellani [:aleca]]

Both dialogs are missing the dynamic stylesheets. Where could also be other dialogs which use the folderTree.

Ah, I wasn't aware of that.
I'll update the patch to be sure to account for those scenarios.

LMK if you want this uplifted to the next beta

For sure, thanks for the heads up.

Comment 22

[Alessandro Castellani [:aleca]]

Attachment: 1637668-folderpane-colours.diff

I've update the patch to account for all the other locations where the Folder Tree is shown.
I can't seem to recreate the error Ruleset ignored due to bad selector. messenger.xhtml:9897:1, even with a fresh new profile.
Any hint on this?

Comment 23

[Richard Marti (:Paenglab)]

Comment on attachment 9149929 [details] [diff] [review]
1637668-folderpane-colours.diff

I still see the warning but can't say what to set to see them. Have you `javascript.options.strict` enabled?

But because they are only warning, we can maybe ignore them.

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 9149929 [details] [diff] [review]
1637668-folderpane-colours.diff

Review of attachment 9149929 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Just some style/truthy/falsy nits

::: mail/base/content/folderPane.js
@@ +1688,5 @@
>      }
> +
> +    // Restore custom icon colors.
> +    for (let i = 0; i < this._rowMap.length; i++) {
> +      let aFolder = this._rowMap[i]._folder;

The a-style naming is for a as in argument.
That's not the case here, and we're also trying to avoid that style completely. 

Better have it just be named "folder".

@@ +2617,5 @@
> +   *
> +   * @param {string} iconColor - The hash color.
> +   */
> +  appendColor(iconColor) {
> +    if (!this.inlineStyle || !iconColor || iconColor == "") {

the last tow here are the same. just check !iconColor

@@ +2838,5 @@
> +      let customColor = this._folder.msgDatabase.dBFolderInfo.getCharProperty(
> +        "folder-icon-color"
> +      );
> +      // Add the property if a custom color was defined for this folder.
> +      if (customColor != "") {

if (customColor)

@@ +3385,5 @@
> +   *
> +   * @param {ftvItem} aFolder - The folder where the color is defined.
> +   * @param {string} newColor - The new hash color to preview.
> +   */
> +  previewSelectedColor(aFolder, newColor) {

please also here just name it folder (to get away from the a-naming)

@@ +3387,5 @@
> +   * @param {string} newColor - The new hash color to preview.
> +   */
> +  previewSelectedColor(aFolder, newColor) {
> +    // If the color is null, it measn we're resetting to the default value.
> +    if (newColor == null || newColor == "") {

if (!newColor)

@@ +3422,5 @@
> +   * inline style in the messenger.xhtml file.
> +   *
> +   * @param {ftvItem} aFolder - The folder where the new color was defined.
> +   */
> +  updateColor(aFolder) {

folder

::: mail/locales/en-US/chrome/messenger/folderProps.dtd
@@ +50,5 @@
>  <!ENTITY folderProps.name.label                  "Name:">
>  <!ENTITY folderProps.name.accesskey              "N">
> +<!ENTITY folderProps.color.label                 "Icon Color:">
> +<!ENTITY folderProps.color.accesskey             "I">
> +<!ENTITY folderProps.reset.button                "Restore default color">

I think this should be named .tooltip, not .button

::: mailnews/base/content/folderProps.js
@@ +8,5 @@
>  var { Gloda } = ChromeUtils.import("resource:///modules/gloda/Gloda.jsm");
>  
>  var gMsgFolder;
>  var gLockedPref = null;
> +var gCurrentColor = null;

or emtpy string maybe?

::: mailnews/base/content/virtualFolderProperties.js
@@ +10,5 @@
>  var msgWindow; // important, don't change the name of this variable. it's really a global used by commandglue.js
>  var gSearchTermSession; // really an in memory temporary filter we use to read in and write out the search terms
>  var gSearchFolderURIs = "";
>  var gMessengerBundle = null;
> +var gCurrentColor = null;

or empty?

@@ +161,5 @@
> +    "folder-icon-color"
> +  );
> +
> +  let colorInput = document.getElementById("color");
> +  colorInput.value = gCurrentColor != "" ? gCurrentColor : gDefaultColor;

this too would be a truthy compare

Comment 25

[Alessandro Castellani [:aleca]]

Attachment: 1637668-folderpane-colours.diff

Thanks for the review.
I fixed everything and launched a try run.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=62b061c64dcab5f3b0a12f1d2ce04ab73ecfa779

Comment 26

[alta88]

Comment on attachment 9150213 [details] [diff] [review]
1637668-folderpane-colours.diff

As predicted elsewhere, this patch was rubber stamped with zero consideration given to perf or architecture.

See this for just the worst of the problems here:
https://searchfox.org/comm-central/source/mail/base/content/folderPane.js#3373

Comment 27

[Alessandro Castellani [:aleca]]

Thanks for the heads up.
I removed the check-in flag, waiting for Magnus to chime in.
Should I rework the patch to use the setFolderCacheProperty?

Meanwhile, alta88, would you be so kind to highlight all the problems you see in this patch, and suggest better approaches?

Comment 28

[Alessandro Castellani [:aleca]]

I did a little test and everything pretty much works the same when using the setFolderCacheProperty method.
The only problem is, obviously, on close the colours are not maintained nor stored anywhere, so everything gets reset.

Do you have any suggestion on how I could persistently store a custom property?
Would using folder.msgDatabase.dBFolderInfo.setCharProperty("folder-icon-color", color); be acceptable, only when a new color is set by the user?

Comment 29

[Magnus Melin [:mkmelin]]

Looks like the problem mentioned in bug 562965. Setting the property when the user does it wouldn't be a problem, only avoid doing direct folder.getCharProperty access

Comment 30

[alta88]

Anyone who wants to touch view code (nsITree related) minimally needs to:

1. Understand how to use a profiler (comments in Bug 1631919 are sad).
2. Get/create several folders, 1 imap, that contain 100K messages each.
   Otherwise, don't touch this extremely hot (startup and general) codepath. There is no QA function in Tb, not even perf tests like with Fx. Let's not replicate the worst of silicon valley practices, ie let the customer test it.

Before you (or I) spend any more time on this advanced view bug, I recommend you graduate by fixing the incorrect cell highlighting on row hover behavior introduced in the Delete column patch. Hint: it would be optimal to do it in core tree layout cpp code, but can be done in js, and even in an extension. Any cell that can perform an action should return feedback, just like a button; basic UX.

As for this bug, there are several issues, the largest is persistent store. Hint: the better architecture would be to store properties in the folderTree.json file already supported. I should think a priority would be to get folder properties out of mork/msf and into a json store. If you need to figure out how to persist json async, see how it's done in session store code.

Comment 31

[Alessandro Castellani [:aleca]]

(In reply to Magnus Melin [:mkmelin] from comment #29)

Looks like the problem mentioned in bug 562965. Setting the property when the user does it wouldn't be a problem, only avoid doing direct folder.getCharProperty access

Yes, I can do that.
I can set the property only when the user saves a new color, and get the property only on first run. For all the other situations (re-rendering, reload, etc.) I will only interact with the Cached value.

(In reply to alta88 from comment #30)

Let's not replicate the worst of silicon valley practices, ie let the customer test it.

I personally have 3 test accounts with 100+ folders, which I know it's not near enough, that's why Magnus and Richard (and other reviewers) test these patches on their machines on multiple platforms, and we also ping contributors and other developers on Matrix to help us test these features. I really don't think that "let the customer test it" is our policy.

Before you (or I) spend any more time on this advanced view bug, I recommend you graduate by fixing the incorrect cell highlighting on row hover behavior introduced in the Delete column patch. Hint: it would be optimal to do it in core tree layout cpp code, but can be done in js, and even in an extension. Any cell that can perform an action should return feedback, just like a button; basic UX.

I kicked off a discussion for this on bug 1639622.

As for this bug, there are several issues

Could you list them, please?

the largest is persistent store. Hint: the better architecture would be to store properties in the folderTree.json file already supported. I should think a priority would be to get folder properties out of mork/msf and into a json store. If you need to figure out how to persist json async, see how it's done in session store code.

Thanks for the info.
This seems a pretty substantial rework, I'll discuss with Magnus and other developers, investigate the possibility of doing it, and define a possible plan of actions.

As a general note
I might be wrong, but the way you write it seems that your tone is very aggressive and patronizing.
I would like to highlight the fact that we're all here to improve Thunderbird, and if we make mistakes we don't do it on purpose and technical feedback and suggestions are always welcomed, but using sentences like "this patch was rubber stamped", or "I recommend you graduate", or "comments in Bug 1631919 are sad", make it sound like you're looking down on me and don't appreciate the work I'm doing.
I would kindly ask you to be more respectful and considerate when writing a comment.
If I totally misread your tone, I apologize sincerely.

Comment 32

[Alessandro Castellani [:aleca]]

Attachment: 1637668-folderpane-colours.diff

Patch updated with the following changes:

• Call the msgDatabase.dBFolderInfo.getCharProperty() only once on first startup.
• Handle all the preview and updates with setFolderCacheProperty and getFolderCacheProperty.
• Call the msgDatabase.dBFolderInfo.setCharProperty() only once on a single folder when the user sets a new color.

Comment 33

[alta88]

(In reply to Alessandro Castellani (:aleca) from comment #31)

If I totally misread your tone, I apologize sincerely.

You did, completely. That's ok, I'm not offended. But even if you had said my code is an atrocity, I wouldn't be offended either; rather, I would go see if you were right. ;)

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 9150525 [details] [diff] [review]
1637668-folderpane-colours.diff

Review of attachment 9150525 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/folderPane.js
@@ +1643,5 @@
>     * Completely discards the current tree and rebuilds it based on current
>     * settings
>     */
>    _rebuild() {
> +    console.log("rebuild");

remove

::: mailnews/base/content/folderProps.js
@@ +191,5 @@
>        "retention.useDefault"
>      ).checked;
>      gMsgFolder.retentionSettings = retentionSettings;
> +
> +    // Check if the icon color was update.

updated.

@@ +213,5 @@
>    }
>  }
>  
> +function folderCancelButton(event) {
> +  if (gMsgFolder) {

doesn't seem we need this check - folderPropsOnLoad checks it for a start, but not later in that function, so it should be safe to assume we have a folder

::: mailnews/base/content/virtualFolderProperties.js
@@ +12,5 @@
>  var gSearchTermSession; // really an in memory temporary filter we use to read in and write out the search terms
>  var gSearchFolderURIs = "";
>  var gMessengerBundle = null;
> +var gCurrentColor = "";
> +var gDefaultColor = "#363959";

we use k for constansts, not g. 
(Or UPPERCASE)

Comment 35

[Alessandro Castellani [:aleca]]

Attachment: 1637668-folderpane-colours.diff

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f8310228f5b2
Implement colour customization option for Folder Pane icons. r=mkmelin, ui-r=paenglab

Comment 37

[Alessandro Castellani [:aleca]]

Should we uplift this for the next 77 beta?
Or it's better to wait a bit to let other devs use it on daily and let it be available on the first 78 beta?

Comment 38

[Magnus Melin [:mkmelin]]

I think no need to uplift. It can just ride the trains.

Comment 39

[alta88]

Comment on attachment 9150838 [details] [diff] [review]
1637668-folderpane-colours.diff


>@@ -1679,16 +1681,42 @@ var gFolderTreeView 
>+    // Restore custom icon colors.
>+    for (let i = 0; i < this._rowMap.length; i++) {
>+      let folder = this._rowMap[i]._folder;
>+      let msgDatabase;
>+      try {
>+        // This will throw an exception if the .msf file is missing,
>+        // out of date (e.g., the local folder has changed), or corrupted.
>+        msgDatabase = folder.msgDatabase;
>+      } catch (e) {}

There is a way to async open/rebuild such a folder, see the feeds code, if it's better not to give up and assume the user won't notice their missing color. But you are opening every folder's db, and until it is closed, it will occupy memory. The folder's db should be closed immediately after getting the property. 
>+
>@@ -2586,16 +2614,32 @@ var gFolderTreeView = {
>+    // Append the inline CSS styling.
>+    this.inlineStyle.textContent += `treechildren::-moz-tree-image(folderNameCol, ${selector}) {fill: ${iconColor};}`;

This method is used in tests for one offs but is not production hygenic; the correct api is sheet.insertRule() and if a rule is cleared then the delete/remove api should be used, I forget which is deprecated.

>+    // If the color is null, it measn we're resetting to the default value.
means

>+    // Append new incline color if defined.
typo?

>diff --git a/mail/base/content/messenger.xhtml b/mail/base/content/messenger.xhtml
>--- a/mail/base/content/messenger.xhtml
>+++ b/mail/base/content/messenger.xhtml
>@@ -39,16 +39,20 @@
>+<!-- NEEDED FOR FOLDER COLOR CUSTOMIZATION -->
>+<?xml-stylesheet href="#inlineStyle" type="text/css"?>
>+<?xml-stylesheet href="#previewStyle" type="text/css"?>

Why is this needed? It throws errors in the devtools style editor. There is already a <style> element and rules are added to its sheet per api. Also applies to same found in dialogs.

Comment 40

[Alessandro Castellani [:aleca]]

(In reply to alta88 from comment #39)

Hey, thank you so much for the detailed review, I'm doing a quick follow up patch to address the issues.

There is a way to async open/rebuild such a folder, see the feeds code

Could you point me to that code?

The folder's db should be closed immediately after getting the property.

I noticed in some sections this is done with folder.msgDatabase = null;, while in others is done with folder.msgDatabase.Close(true);.
Which one is the correct way to handle this?

@@ -2586,16 +2614,32 @@ var gFolderTreeView = {

• // Append the inline CSS styling.
• this.inlineStyle.textContent += treechildren::-moz-tree-image(folderNameCol, ${selector}) {fill: ${iconColor};};
  This method is used in tests for one offs but is not production hygenic; the correct api is sheet.insertRule() and if a rule is cleared then the
  delete/remove api should be used, I forget which is deprecated.

+<!-- NEEDED FOR FOLDER COLOR CUSTOMIZATION -->
+<?xml-stylesheet href="#inlineStyle" type="text/css"?>
+<?xml-stylesheet href="#previewStyle" type="text/css"?>
Why is this needed? It throws errors in the devtools style editor. There is
already a <style> element and rules are added to its sheet per api. Also
applies to same found in dialogs.

I'm handling the colour customization via inline style and not by updating an existing CSS file.
This approach allows me quickly overwrite the previously written CSS, especially when dealing with the preview color, without the necessity of constantly calling insertRule() and deleteRule().
Writing inline CSS seemed more straightforward to handle.

I'll take a look on how to refactor this to use the CSSStyleSheet API and its potential shortcomings.

Comment 41

[alta88]

(In reply to Alessandro Castellani (:aleca) from comment #40)

(In reply to alta88 from comment #39)

Hey, thank you so much for the detailed review, I'm doing a quick follow up patch to address the issues.

There is a way to async open/rebuild such a folder, see the feeds code

Could you point me to that code?

Trace isMsgDatabaseOpenable().

The folder's db should be closed immediately after getting the property.

I noticed in some sections this is done with folder.msgDatabase = null;, while in others is done with folder.msgDatabase.Close(true);.
Which one is the correct way to handle this?

The former is used in feeds, with potentially numerous folders opened/closed in one update cycle, without any issue I know of; you can look at the code but I don't recall any practical difference.

@@ -2586,16 +2614,32 @@ var gFolderTreeView = {

• // Append the inline CSS styling.
• this.inlineStyle.textContent += treechildren::-moz-tree-image(folderNameCol, ${selector}) {fill: ${iconColor};};
  This method is used in tests for one offs but is not production hygenic; the correct api is sheet.insertRule() and if a rule is cleared then the
  delete/remove api should be used, I forget which is deprecated.

+<!-- NEEDED FOR FOLDER COLOR CUSTOMIZATION -->
+<?xml-stylesheet href="#inlineStyle" type="text/css"?>
+<?xml-stylesheet href="#previewStyle" type="text/css"?>
Why is this needed? It throws errors in the devtools style editor. There is
already a <style> element and rules are added to its sheet per api. Also
applies to same found in dialogs.

I'm handling the colour customization via inline style and not by updating an existing CSS file.
This approach allows me quickly overwrite the previously written CSS, especially when dealing with the preview color, without the necessity of constantly calling insertRule() and deleteRule().
Writing inline CSS seemed more straightforward to handle.

The <style> element is inline, all you need is one, not 4 (tagColors.css could have been inline too rather than an empty file but there is some value in the documentation aspect). If a color is previewed it is element.sheet.insertRule()d and added to folder cache. If the user accepts, it is stored; if declined, the rule is removed based on the index used to insert and cache entry is removed. Can't be any simpler than that.

It's subpar that the json file isn't being used for this, it's not that hard.

Comment 42

[Alessandro Castellani [:aleca]]

Attachment: 1637668-folderpane-colours-PART2.diff

Here's a follow up to improve some aspects of this feature.
Changes introduced in this patch:

• Close the msgDatabase immediately after getting the custom color to prevent memory leak.
• Use an empty folderColors.css file to save the currently stored custom colors with css.insertRule().
• Some typos fixes.

I left the previewStyle as inline file in referenced via ID because using insertRule() and deleteRule() when a new color is picked is painfully slow. The color picker was lagging and CPU usage rising.
I guess those methods are not really performant when dealing with such a fast value update like moving the color picker.

Comment 43

[alta88]

Comment on attachment 9152198 [details] [diff] [review]
1637668-folderpane-colours-PART2.diff

What I meant is that you should have 1 <html:style id="folderColorsStyle"> element only. This element has a |sheet| property, with insertRule() etc methods and the rules themselves. It is used for both preview and existing rules. Iterating all document.styleSheets is, as you found out, very bad and unnecessary since you can get the right sheet directly from the inline style element. When you preview, you insertRule(rule, index). If canceled/changed, you deleteRule(index) and insertRule(newrule, index).

Comment 44

[Alessandro Castellani [:aleca]]

(In reply to alta88 from comment #43)

What I meant is that you should have 1 <html:style id="folderColorsStyle"> element only

Now I got it, thanks.
I previously had the <?xml-stylesheet href="folderColorsStyle"?> because I couldn't get that ID directly, and I thought it was a limitation of XUL + HTML, but it seems to work normally now.

When you preview, you insertRule(rule, index). If canceled/changed, you deleteRule(index) and insertRule(newrule, index).

I did that while updating the patch, but it's not possible because the preview is updated at every color change when the color picker is touched, which might mean hundreds of changes per second if the user quickly moves the picker around the color wheel.
We need that kind of accuracy to ensure a proper real time preview of the selected color, and constantly calling insertRule and deleteRule at every value change has some serious performance issues.

Comment 45

[Alessandro Castellani [:aleca]]

Attachment: 1637668-folderpane-colours-PART2.diff

Comment 46

[alta88]

(In reply to Alessandro Castellani (:aleca) from comment #44)

When you preview, you insertRule(rule, index). If canceled/changed, you deleteRule(index) and insertRule(newrule, index).

I did that while updating the patch, but it's not possible because the preview is updated at every color change when the color picker is touched, which might mean hundreds of changes per second if the user quickly moves the picker around the color wheel.
We need that kind of accuracy to ensure a proper real time preview of the selected color, and constantly calling insertRule and deleteRule at every value change has some serious performance issues.

That's because the event listener here should only be "change" on a type="color" input, fired when dialog is accepted/canceled. The color changes both on the wheel and the colorpicker box, so it's not like the user can't preview what it looks like. And the existing method pegs cpu when moving the picker regardless; it seems not worth the feature of also updating the folder in the tree.. Especially since invalidateRow() may also invalidate() the entire tree (invalidateCell() doesn't work on only a cell in my experience).

Comment 47

[Magnus Melin [:mkmelin]]

Comment on attachment 9152235 [details] [diff] [review]
1637668-folderpane-colours-PART2.diff

Review of attachment 9152235 [details] [diff] [review]:
-----------------------------------------------------------------

Only updating on "change" seems good to me.

Comment 48

[Alessandro Castellani [:aleca]]

That's because the event listener here should only be "change" on a type="color" input, fired when dialog is accepted/canceled
Only updating on "change" seems good to me.

I disagree.
The color must change when the user interacts with the color wheel.
Asking the user to open the picker, pick a color, close the picker, check if the color looks good, open the picker again to change it, and so on, it's terrible UX.
Furthermore, interacting with the color wheel without seeing any visual feedback of the picked color will most likely result in letting the user think that the feature is broken/buggy.

I'll investigate the invalidateRow() you pointed out, but for what I can see it's used a lot to update the display of a specific row and it doesn't leak to the entire tree.

Comment 49

[alta88]

Comment on attachment 9152235 [details] [diff] [review]
1637668-folderpane-colours-PART2.diff

Review of attachment 9152235 [details] [diff] [review]:
-----------------------------------------------------------------

> The color must change when the user interacts with the color wheel.

No. First, there is no expectation of instant apply here; that happens only to the picker launch box color. How do we know? Because the wheel dialog has accept and cancel buttons. Second, and far more importantly, the tree widget simply does not support such niceties. It is Far Worse UX to listen to a computer fan max out when mouse moving the colorpicker wheel. It demonstrates a flaw in the application and makes it look bad.

Comment 50

[Alessandro Castellani [:aleca]]

(In reply to alta88 from comment #49)

No. First, there is no expectation of instant apply here; that happens only to the picker launch box color.

That's visually and intuitively incorrect.
If the user needs to edit the color of an element, he needs to see the preview applied in real time to make an educated decision, without the necessity of closing and reopening the picker multiple times. That's a frustrating interaction.

How do we know? Because the wheel dialog has accept and cancel buttons.

Those buttons are there to let the user apply the color or discard the changes, and they're not in any way related to the necessity of a preview applied in context.

It is Far Worse UX to listen to a computer fan max out when mouse moving the colorpicker wheel.
It demonstrates a flaw in the application and makes it look bad.

The CPU usage increase happens only if you interact very aggressively with the wheel non-stop.
A "normal" interaction doesn't provoke anything and the UI doesn't lag or shows issues.

I agree that this is not optimal, but there are multiple applications that when interacting on some specific areas cause the CPU to spike for a few seconds, and considering that this is a very particular feature which will be used very sporadically, it can't affect the performance of the application at all.

Comment 51

[alta88]

Comment on attachment 9150838 [details] [diff] [review]
1637668-folderpane-colours.diff

Now we finally come to the most serious errors in this patch.

>@@ -1679,16 +1681,42 @@ var gFolderTreeView = {
>+    // Restore custom icon colors.
>+    for (let i = 0; i < this._rowMap.length; i++) {

1. It is the most basic concept of infinite lists that only rows in the viewport are processed. You are potentially opening dozens or hundreds of possibly very large folders, if the user leaves folders expanded, in a startup path no less, to paint (perhaps) a color on a row that may not be viewed for a very long time (ever).

2. This restoring is being done in _rebuild() and it should have been traced out who and when it is called. Because when it is, the whole thing is done again, thus failing to understand why the property is being cached in the first place and leveraging that. And even then, it's not clear if the color is actually applied. Hint: every time the folder view picker changes, for one, this runs.

I'm not going to spend any more free time on this. Magnus, I suggest you back it out until it can be properly mentored. It is really rather disappointing that this landed.

Comment 52

[alta88]

(In reply to Alessandro Castellani (:aleca) from comment #50)

I agree that this is not optimal

Then why not fire on mouseup. On input is just wrong.

Comment 53

[Alessandro Castellani [:aleca]]

(In reply to alta88 from comment #51)

@@ -1679,16 +1681,42 @@ var gFolderTreeView = {

• // Restore custom icon colors.
• for (let i = 0; i < this._rowMap.length; i++) {

1. It is the most basic concept of infinite lists that only rows in the viewport are processed.

We need to go through all the folders at first run to cache and apply the color if present.
This looping mechanism is already used in the Address Book to fetch the index of a folder, and in the FolderPane itself to restore the open state of each folder.
I didn't add the fetching of the colors in that method because that's called various times throughout the life of the application, instead _rebuild is called only once.

2. This restoring is being done in _rebuild() and it should have been traced out who and when it is called.

The _rebuild is called only at first launch, or when you setup a new account, or when you delete an account, basically when the app needs to rebuild the entire tree from scratch. Is not called when you add a folder, or when you delete a folder, or in any other type of interaction.

Hint: every time the folder view picker changes, for one, this runs.

That's absolutely not true.

Then why not fire on mouseup. On input is just wrong.

That's completely useless.
The event listener is attached to the button, NOT the color wheel. A mouse up on the button would be triggered only when the user clicks on the color button.

Comment 54

[alta88]

(In reply to Alessandro Castellani (:aleca) from comment #53)

(In reply to alta88 from comment #51)

@@ -1679,16 +1681,42 @@ var gFolderTreeView = {

• // Restore custom icon colors.
• for (let i = 0; i < this._rowMap.length; i++) {

1. It is the most basic concept of infinite lists that only rows in the viewport are processed.

We need to go through all the folders at first run to cache and apply the color if present.

Nope, that's a wrong design. Period. Maybe you will understand once you realize Bug 1641950 can't be fixed now without an overhaul.

This looping mechanism is already used in the Address Book to fetch the index of a folder, and in the FolderPane itself to restore the open state of each folder.
I didn't add the fetching of the colors in that method because that's called various times throughout the life of the application, instead _rebuild is called only once.

Nope, that's wrong. Are you aware _rebuild() can be called simply by a new message arriving.

2. This restoring is being done in _rebuild() and it should have been traced out who and when it is called.

The _rebuild is called only at first launch, or when you setup a new account, or when you delete an account, basically when the app needs to rebuild the entire tree from scratch. Is not called when you add a folder, or when you delete a folder, or in any other type of interaction.

Hint: every time the folder view picker changes, for one, this runs.

That's absolutely not true.

Had you set a breakpoint or added a log in the debugger, this statement would not now have to be retracted.

Then why not fire on mouseup. On input is just wrong.

That's completely useless.
The event listener is attached to the button, NOT the color wheel. A mouse up on the button would be triggered only when the user clicks on the color button.

Yes, of course mouseup would be added to the wheel.

Comment 55

[Alessandro Castellani [:aleca]]

(In reply to alta88 from comment #54)

We need to go through all the folders at first run to cache and apply the color if present.

Nope, that's a wrong design. Period. Maybe you will understand once you realize Bug 1641950 can't be fixed now without an overhaul.

Interesting, so the tree doesn't list all the folders in the array but only those visible, which is good for performance.
Maybe to fix that would be better to detect a custom color when the open state of the parent folder is toggled open, instead of looping through all the non visible folders.

I didn't add the fetching of the colors in that method because that's called various times throughout the life of the application, instead _rebuild is called only once.

Nope, that's wrong. Are you aware _rebuild() can be called simply by a new message arriving.

That's incorrect.
The _rebuild method is called only if the folder structure changes in these scenarios:

• On first launch.
• When the display mode changes.
• When a new unread Folder comes from the server.
• When the user changes the Favourite status of a Folder.

These scenarios are very unique and not commonly happening in the lifecycle of the application, and even if these were happening more often we would always need to apply the cached colors because the entire folder tree have been rebuilt.

Hint: every time the folder view picker changes, for one, this runs.

That's absolutely not true.

Had you set a breakpoint or added a log in the debugger, this statement would not now have to be retracted.

Yes, I did, and also set a stack trace to see from where that method is being called.
Did I miss something? Would you be able to provide a log if my statement is incorrect?

Then why not fire on mouseup. On input is just wrong.

That's completely useless.
The event listener is attached to the button, NOT the color wheel. A mouse up on the button would be triggered only when the user clicks on the color button.

Yes, of course mouseup would be added to the wheel.

How? As far as I know that's not possible.

Comment 56

[Alessandro Castellani [:aleca]]

Attachment: 1637668-folderpane-colours-PART2.diff

Another round of improvements.

• Use the looping already happening on first build in the _restoreOpenState() method and remove the unnecessary extra for loop.
• Account for the custom colors of hidden subfolders. bug 1641950
• Prevent opening the folder DB and applying the cached property if already present, in case of a rebuild happening after first run.
• Avoid closing the folder DB for those folders that need it open, therefore preventing the regeneration of the DB when needed.

Comment 57

[Magnus Melin [:mkmelin]]

Comment on attachment 9153635 [details] [diff] [review]
1637668-folderpane-colours-PART2.diff

Review of attachment 9153635 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like an improvement. r=mkmelin

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/56bf1f08c686
Follow up improvements to Folder Pane icons colour customization. r=mkmelin DONTBUILD

Comment 60

[Alessandro Castellani [:aleca]]

Comment on attachment 9153635 [details] [diff] [review]
1637668-folderpane-colours-PART2.diff

[Approval Request Comment]
User impact if declined: This follow up patch cleans up a lot unnecessary duplication in the inline CSS files, removes an extra unnecessary for loop on startup, prevents reapplying colors when the tree is rebuilt, and fixes an issue where custom colors are not applied if the folder is hidden inside a collapsed parent.

Comment 61

[Wayne Mery (:wsmwk)]

Comment on attachment 9153635 [details] [diff] [review]
1637668-folderpane-colours-PART2.diff

Approved for beta

Comment 62

[Geoff Lankow (:darktrojan)]

Thunderbird 78.0b2 (second part, first was already there):
https://hg.mozilla.org/releases/comm-beta/rev/c8d94a2edc42e0e8d552f2a71b2cde469dc10f87

Comment 64

[Jean BUSSER]

No problems for me.
This version suits me perfectly
Many thanks.