764242 - magicCopy does not respect clipboard.autocopy pref

Status: REOPENED

(Thunderbird :: Instant Messaging, defect)

Description

[:Mook]

See URL; setting clipboard.autocopy to false does not disable auto-copy when messenger.conversations.selections.magicCopyEnabled is true.

Comment 1

[:Mook]

Attachment: chat: obey clipboard.autoCopy being disabled in magicCopy

Huh. I had most of the patch sitting around for some reason; must have forgotten about it halfway through trying to build Mozilla (again).

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 778294 [details] [diff] [review]
chat: obey clipboard.autoCopy being disabled in magicCopy

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

Thanks for looking into scratching this itch of yours :-).

::: chat/content/convbrowser.xml
@@ +139,2 @@
>        <field name="magicCopyInitialized">false</field>
> +      

Nit: Adding trailing whitespace isn't the best idea ;).

@@ +733,5 @@
>                  this.contentWindow.controllers.insertControllerAt(0, this);
>  
> +                if (this.autoCopyEnabled &&
> +                    Services.clipboard.supportsSelectionClipboard())
> +                {

Nit: The { should stay at the end of the previous line.

@@ +741,5 @@
>                  }
>                }
>                if (!this.magicCopyInitialized) {
>                  Services.prefs.addObserver(this.magicCopyPref, this, false);
> +                Services.prefs.addObserver(this.autoCopyPref, this, false);

Observing this pref on OSes where Services.clipboard.supportsSelectionClipboard() is false seems unfortunate; is it possible to avoid it?

Also, I'm not sure observing changes to this preference is really worth it: it looks like the only place where this pref is used in gecko (http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#354) doesn't set observers, so I assume you need to restart any XUL app for changes to this pref to take effect.
How would you feel about just checking the pref value when we are about to add the selection listener, and keeping a boolean indicating if we set a selection listener or not (so that we don't check the pref again at uninitialization, as its value may have changed...).

@@ +1069,5 @@
>  
>            // and our selection listener!
> +          if (this.autoCopyEnabled &&
> +              Services.clipboard.supportsSelectionClipboard())
> +          {

Nit: The { should stay at the end of the previous line.

Comment 3

[:Mook]

Attachment: Only read the pref once

Thanks for the review!

Given that the pref is normally read there anyway (that is: from experimentation, without magicCopy, I can only get it to recognize my changes when I open a new conversation): Changed so we just read the pref once, and keep the same value thereafter.  This actually means the patch can be a lot more minimal now - just check the cached value instead of whether we have support for a primary selection.

The docshell swapping code looks suspicious, though... I'm not convinced it's tearing down and restoring *both* sides correctly.  But that's not limited to the copy stuff, so I'm leaving that alone for now.

Comment 4

[Florian Quèze [:florian]]

(In reply to :Mook from comment #3)

> The docshell swapping code looks suspicious, though... I'm not convinced
> it's tearing down and restoring *both* sides correctly.

I'm pretty sure it's broken, as we had to fix it: https://bugzilla.instantbird.org/show_bug.cgi?id=1940

Comment 5

[aleth [:aleth]]

Porting this patch to Instantbird https://bugzilla.instantbird.org/show_bug.cgi?id=2062. The fix will be in TB after the next merge.

Comment 6

[aleth [:aleth]]

As per comment 5, this is now fixed upstream.

Comment 7

[Patrick Cloke [:clokep]]

This is not fixed in Thunderbird, therefore this should stay open until the next merge.

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 778811 [details] [diff] [review]
Only read the pref once

Removing the review request as I think we agreed that the way forward is to merge chat/ again between Instantbird and Thunderbird.