Closed Bug 1388731 Opened 2 years ago Closed 2 years ago

Use a shared in themes to simplify changes


(Thunderbird :: Theme, enhancement)

Not set


(Not tracked)

Thunderbird 57.0


(Reporter: Paenglab, Assigned: Paenglab)



(1 file)

I'm planning to split the combined toolbar SVG into separated SVGs like FX has done also for performance reasons. With our actual files we have to add/change the shared files in every platform for it's own. It would be simpler if we could do this in a shared and will be included in the platform ones.
Moved the shared files to shared/

I also moved the communicator and editor blocks to the end when they aren't already there to make the files a bit more consistent and cleaned up over the platforms. In a future bug we could also sort them alphabetically.
Assignee: nobody → richard.marti
Attachment #8895361 - Flags: review?(jorgk)
Tried the patch on all platforms and it looks all is working as it should.
I'm a little bit confused by this patch.

You're mixing two things, the shared stuff, and moving stuff around. I don't want to be a pain, but that should really be two patches.

As for the shared stuff:

You create 32 lines in, 27 for all platforms, 1 for Linux/Windows, 4 for Mac.
However, the three platforms don't have all those:
Linux has 26, Windows 28 and Mac 31.

So Linux is getting something it didn't have before, counted with
  grep -i shared linux\  | wc -l

The other thing I find a little strange is that a file that supposedly contains "shared" things, has an #ifdef (or #ifndef) to exclude things which aren't shared after all. I'm willing to accept that, but I'd like you to check what's going on with Linux and why that had 26 shared things before and now it's getting 28.
The two added files are toolbarbutton-arrow@2x.png and toolbarbutton-arrow-inverted@2x.png. Until now Linux had almost no HiDPI support but now Gnome, KDE etc. have it now and I'm planning to use this files for this.
Comment on attachment 8895361 [details] [diff] [review]

OK, I did the bean counting for the shared stuff, so I hope you got stuff right that you moved around ;-(
Attachment #8895361 - Flags: review?(jorgk) → review+
Oh, I forgot to say: Next time please make it easier for the reviewer and split unrelated things into multiple patches.
My local builds on all platform looked good. So I should have moved all correctly.
Keywords: checkin-needed
(In reply to Richard Marti (:Paenglab) from comment #7)
> My local builds on all platform looked good. So I should have moved all
> correctly.
And you tried do insert an image and check the alignment icon? ;-)
Anyway, I checked patch again and counted the lines in the moved blocks, all Swiss quality :-)
Pushed by
Unify the shared elements in a shared r=jorgk
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 57.0
You need to log in before you can comment on or make changes to this bug.