Closed Bug 1571687 Opened 8 months ago Closed 7 months ago

move/copy the parts of editor that are used by thunderbird to mail/components/compose

Categories

(Thunderbird :: Message Compose Window, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: mkmelin, Assigned: khushil324)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 8 obsolete files)

comm/editor contains a mishmash of code partly used by the Thunderbird compose window, and partly (used to be) used by the defunct standalone editor.

de-grid requires some work in this directory, and also a lot of upcoming rewrites would be easier to overview if the code would be put into the real place it's used. We're not going to do any work on code we're not using.

In short, it seems to make sense to go through the files we use and copy them to mail/components/compose, and then just abandon the editor dir. Having an editor named dir was not a great idea to begin with since there is the m-c editor directory too.

There are some ifdefs for which files we actually use:
https://searchfox.org/comm-central/source/editor/ui/jar.mn

I would move the composer only parts to suite. This already bugged me for some time. Most of editor is shared with suite so I would take it to mailnews. Suite is currently not working so you can more or less do with the editor part as needed. If we make it workable again we need to adapt it anyway to remove the current code duplication.

Attachment #9087405 - Flags: review?(mkmelin+mozilla)
Attachment #9087405 - Attachment is obsolete: true
Attachment #9087405 - Flags: review?(mkmelin+mozilla)

You created duplicate files, so packaging fails:
[task 2019-08-22T14:23:53.789Z] 14:23:53 INFO - package> WARNING: Found 71 duplicated files taking 548165 bytes (uncompressed)
[task 2019-08-22T14:23:53.789Z] 14:23:53 INFO - package> ERROR: The following duplicated files are not allowed:
[task 2019-08-22T14:23:53.789Z] 14:23:53 INFO - package> chrome/comm/content/editor/images/tag-anchor.gif
[task 2019-08-22T14:23:53.789Z] 14:23:53 INFO - package> chrome/messenger/content/messenger/messengercompose/images/tag-anchor.gif
[task 2019-08-22T14:23:53.789Z] 14:23:53 INFO - package> chrome/comm/content/editor/EdLabelProps.js
[task 2019-08-22T14:23:53.789Z] 14:23:53 INFO - package> chrome/messenger/content/messenger/messengercompose/EdLabelProps.js
[task 2019-08-22T14:23:53.789Z] 14:23:53 INFO - package> chrome/comm/content/editor/TeXZilla.js
[task 2019-08-22T14:23:53.789Z] 14:23:53 INFO - package> chrome/messenger/content/messenger/messengercompose/TeXZilla.js

You could add them to allowed-dupes.mn as a temporary measure. Why copy ComposerCommands.js, isn't that for the SeaMonkey HTML editor? Or we use parts of it?

If they are being copied from editor/ui, why not use hg copy?
The code in editor/ui is still used by the SeaMonkey HTML composer/editor so be careful what you do with the files in there.

(In reply to Ian Neal from comment #5)

The code in editor/ui is still used by the SeaMonkey HTML composer/editor so be careful what you do with the files in there.

editor/ui code is as it is. Nothing is changed there.

(In reply to Jorg K (GMT+2) from comment #4)

You could add them to allowed-dupes.mn as a temporary measure. Why copy ComposerCommands.js, isn't that for the SeaMonkey HTML editor? Or we use parts of it?

Some TB related code also lays there.

Why not use hg copy as Ian suggested? That way you get the history.

Please use hg copy.
You didn't change editor/ui/jar.mn? I'd think we don't need anything from that the editor/ directory after this. Not sure exactly how we skip that directory entirely.

Some files are not really needed in practice, so unclear if we should bring those across. Like EdFormProps.xul

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

Some files are not really needed in practice, so unclear if we should bring those across. Like EdFormProps.xul

There are few. But I am planning it to remove during the de-grid task because currently, it will be little chaotic.

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

Please use hg copy.

Sure.

You didn't change editor/ui/jar.mn? I'd think we don't need anything from that the editor/ directory after this. Not sure exactly how we skip that directory entirely.

Nope, everything in editor directory is as it is.

That was my point, it shouldn't I think? Aren't then the editor/ files still packaged with Thunderbird?

Attachment #9087694 - Flags: review?(mkmelin+mozilla)
Attachment #9087694 - Attachment is obsolete: true
Attachment #9088095 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED

What's the best way to handle hg copying a bunch of dtds over from editor/ to mail/ without making localizers do any additional work? Can we provide a script, or just the commands? Or just post to the l10n group and let them know they can copy over the files?

Flags: needinfo?(l10n)

Localizers generally don't have access to hg anymore, so if you want to copy files around, you'll need to do it for them.

That said, it'd be a good idea to post to .l10n and explain what's becoming of the editor/ui strings.

Flags: needinfo?(l10n)

Alright, Khushil, can you add a bash script that would loop for each of the locales, and do appropriate hg copies?
The l10 repo is https://hg.mozilla.org/l10n-central/.

Something like
for dir in find . -mindepth 1 -type d; do
hg --cwd=$dir cp editor/ui/chrome messenger/messengercompose
done

... or whatever the right path is

markdown changed that of course... but it's a tick each side of the find command

Attached file Bug-1571687_copy-editor-locales.bash (obsolete) —
Attachment #9089032 - Flags: feedback?(mkmelin+mozilla)
Attachment #9089032 - Attachment mime type: application/octet-stream → text/plain

Posted to mozilla.dev.l10n.

Comment on attachment 9088095 [details] [diff] [review]
Bug-1571687_move-editor-parts-to-components-compose.patch

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

Please rebase so this applies on tip
Attachment #9088095 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9089032 [details]
Bug-1571687_copy-editor-locales.bash

Please add a check that dir has an editor subdir too. 

if [ -d "$dir/editor" ]; then
   hg --cwd=$dir .........
fi

I think you could also do this, and then let the caller pass the */ or ./ on command line:

for dir in $@; do
Attachment #9089032 - Flags: feedback?(mkmelin+mozilla)

Please rebase so this applies on tip

And wait for bug 1578300 which will mess in editor.

Also there is bug 1577443 which made changes to editor/ui/composer/content/ComposerCommands.js

Attachment #9088095 - Attachment is obsolete: true
Attachment #9090468 - Flags: review?(mkmelin+mozilla)
Attached file Bug-1571687_copy-editor-locales.bash (obsolete) —
Attachment #9089032 - Attachment is obsolete: true
Attachment #9090473 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9090473 [details]
Bug-1571687_copy-editor-locales.bash

you forgot to set the dir for hg
Comment on attachment 9090468 [details] [diff] [review]
Bug-1571687_move-editor-parts-to-components-compose.patch

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

Seems ok.
Attachment #9090468 - Flags: review?(mkmelin+mozilla) → review+

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

Comment on attachment 9090473 [details]
Bug-1571687_copy-editor-locales.bash

you forgot to set the dir for hg

It's also not committing and pushing, which I would expect such script to do.

(In reply to Francesco Lodolo [:flod] from comment #30)

It's also not committing and pushing, which I would expect such script to do.

Let me update the script.

Attached file Bug-1571687_copy-editor-locales.bash (obsolete) —
Attachment #9090473 - Attachment is obsolete: true
Attachment #9090473 - Flags: feedback?(mkmelin+mozilla)
Attachment #9090716 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9090716 [details]
Bug-1571687_copy-editor-locales.bash

I don't think you need (or should use --after), that's if the file already exists, but here it doesn't.
But other than that, it looks ok.

Flod, would you be running the script, or who?
Attachment #9090716 - Flags: feedback?(francesco.lodolo)

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

Flod, would you be running the script, or who?

Is :fallen still in charge of Thunderbird l10n?

But yes, it's probably on me to run it, since I'll need to disable Pontoon's sync temporarily.

He's the one that knows it the best yes.

Comment on attachment 9090716 [details]
Bug-1571687_copy-editor-locales.bash

Don't really need the `--after` or  the `hg add`, since you're copying an existing file with `cp`.
Attachment #9090716 - Flags: feedback?(francesco.lodolo) → feedback+
Attachment #9090716 - Attachment is obsolete: true
Attachment #9090716 - Flags: review?(mkmelin+mozilla)
Attachment #9090980 - Flags: feedback+
Keywords: checkin-needed

Can we clarify a few things first.

Why are there two new duplicate JS files?

+chrome/messenger/content/messenger/messengercompose/TeXZilla.js
+chrome/comm/content/editor/TeXZilla.js
+chrome/messenger/content/messenger/messengercompose/EdAEAttributes.js
+chrome/comm/content/editor/EdAEAttributes.js

I'd expect a whole lot more, or none at all.

What happened to the idea of not building editor/ui at all?
https://searchfox.org/comm-central/search?q=editor%2Fui&case=false&regexp=false&path=mail
The whole thing appears to be lacking all the build aspects.

And what do we do with Bug-1571687_copy-editor-locales.bash?

Flags: needinfo?(mkmelin+mozilla)
Keywords: checkin-needed
Comment on attachment 9090468 [details] [diff] [review]
Bug-1571687_move-editor-parts-to-components-compose.patch

Seems to be missing the build parts. I might be wrong.
Attachment #9090468 - Flags: feedback-

I am updating it and sending to try-run. Submitting the patch after try-run.
And bash file is for the Localization Team.

Sure, but what about the build parts? I've discussed it with Magnus on Monday and it appeared that he wanted to stop building in editor/ui which is the correct way to go. And also please explain the additional allowed duplicates.

Yes we want to stop building that, but we can take it one step at a time.
Maybe removing this would do it? https://searchfox.org/comm-central/rev/7a16820ae8c33db28e912940437995f360bcfffb/mail/app.mozbuild#29
Would have to try with that and a removed editor/ dir. Probably more places too.

Flags: needinfo?(mkmelin+mozilla)

Yes, I am updating the patch with the required changes and will send it for try-run.

Comment on attachment 9091427 [details] [diff] [review]
Bug-1571687_move-editor-parts-to-components-compose.patch

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

Looks good. r=mkmelin
Attachment #9091427 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
Comment on attachment 9091427 [details] [diff] [review]
Bug-1571687_move-editor-parts-to-components-compose.patch

Please undo the changes to l10n.toml, the file needs to work in cross-channel, and we still need editor on older branches.
Attachment #9091427 - Flags: review-
Keywords: checkin-needed
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2c47346623c4
copy the parts of editor that are used by Thunderbird to mail/components/compose. r=mkmelin,pike

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
Blocks: 1628795
Blocks: 1628799
You need to log in before you can comment on or make changes to this bug.