Closed Bug 1448946 Opened 3 years ago Closed 3 years ago

Remove remaining editor overlays

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 61.0

People

(Reporter: jorgk-bmo, Assigned: Paenglab)

References

Details

Attachments

(4 files)

In bug 1448657 some easy unused ones were removed, that leaves the hard ones :-(

This is a list, maybe incomplete:
EdColorPropsOverlay.xul
EdImageOverlay.xul
EdImageOverlayOverlay.xul
mailComposeEditorOverlay.xul
EdSpellCheckOverlay.xul
It is incomplete, EdDialogOverlay.xul for example overlays over 20 files. The overlaying in editor is a mess.
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f466d0abed9f
remove trailing spaces in editor. rs=white-space-only
This patch removes all editor overlays TB uses. There are still overlays for SM which I haven't touched because I can't check them.

Some dialogs had the EdDialogOverlay.xul referenced althought they haven't used it. So I removed the overlay.

There where also some files which are packaged for TB but never used. I moved them  to SM in jar.mn. Thanfully Jörg removed the trailing white spaces in the editor files and you can see now what i changed in the files.

Frank-Rainer, please can you check that I haven't broken SM more that it is?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8964117 - Flags: review?(jorgk)
Attachment #8964117 - Flags: review?(frgrahl)
Composer had a overlay which added some menus and format buttons. Now the menus and commandsets/keysets directly to messengercompose.xul. The format buttons are in a include file.

I also removed some CSS which isn't used in TB and was forgotten to remove.

Try build with both patches: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1e3064ad1f6fdd2922a68c655826ae2435471a6c

If there are failures, they come from the second patch as a try with only the first patch was successful ( https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3f1d1101aaea0bfd75b6d7884f7cf0b800811f85 ).
Attachment #8964118 - Flags: review?(jorgk)
This might be a little easier to look at since the code in
mailComposeEditorOverlay.xul -> EdImageLinkLoader.js
doesn't show up as a huge difference.
Attachment #8964117 - Flags: review?(jorgk) → review+
Comment on attachment 8964118 [details] [diff] [review]
composeOverlay.patch

This comment applies to both patches.

This is very hard to review, huge patches, lots of reshuffling. In part we need to rely on the patch author who has a lot of experience in this field. On the other hand, I tried to find a malfunction in the HTML editor and didn't find anything.

So thanks a lot for this HUGE effort!!!

Oh, funny that onAccept() from EdColorProps.js had three copies ;-)

How about landing this? With all due respect, I don't think FRG can give a detailed review here and I assume that SM's mail part is not working well enough to test this. I'd like to see this land soon since overlay support will be gone from M-C in a matter of days, perhaps a week.
Attachment #8964118 - Flags: review?(jorgk) → review+
Keywords: leave-open
Comment on attachment 8964117 [details] [diff] [review]
editorOverlay.patch

This part is currently broken in SeaMonkey so I can't test it.

You renamed mailnews/compose/content/mailComposeEditorOverlay.xul to editor/ui/dialogs/content/EdImageLinkLoader.js but still keep 

> chrome://messenger/locale/messengercompose/mailComposeEditorOverlay.dtd

Shouldn't the dtd be renamed for consistency too?
Attachment #8964117 - Flags: review?(frgrahl) → review+
Thanks. Renaming the DTD files leads to create a complete new file for the localizers. So it's better to leave them like they are.
Keywords: checkin-needed
> So it's better to leave them like they are.

You could put a note mentioning the original file in the dtd.
Long discussion on this here:
https://groups.google.com/d/msg/mozilla.dev.l10n/yCA_4qiJWGc/BIimkRiaAgAJ

We learned from our errors, renaming the files will cause retranslations and even the note could be overlooked.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1ff14a05b2a1
Replace the Editor overlays with inlining and pre-processing. r=jorgk,frg
https://hg.mozilla.org/comm-central/rev/3213fa235aef
Replace editorOverlay.xul with inlining and pre-processing. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 61.0
Attachment #8965266 - Flags: review?(jorgk)
Comment on attachment 8965266 [details] [diff] [review]
Remove trailing empty lines from editor/ui/ .dtd-files

OK, thanks, maybe better landed as part of bug 1399756? I'll take care of it.
Attachment #8965266 - Flags: review?(jorgk) → review+
You need to log in before you can comment on or make changes to this bug.