Last Comment Bug 463003 - Context menu for selected text in content shows all items
: Context menu for selected text in content shows all items
Status: RESOLVED FIXED
: dev-doc-complete, regression
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Thunderbird 3.0b1
Assigned To: Phil Ringnalda (:philor)
:
Mentors:
Depends on:
Blocks: 461112
  Show dependency treegraph
 
Reported: 2008-11-04 00:40 PST by Phil Ringnalda (:philor)
Modified: 2010-10-19 10:35 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix v.1 (1.77 KB, patch)
2008-11-04 00:40 PST, Phil Ringnalda (:philor)
no flags Details | Diff | Splinter Review
Fix v.2 (4.26 KB, patch)
2008-11-05 00:34 PST, Phil Ringnalda (:philor)
standard8: review+
Details | Diff | Splinter Review

Description Phil Ringnalda (:philor) 2008-11-04 00:40:12 PST
Created attachment 346224 [details] [diff] [review]
Fix v.1

More fallout from my worst-ever job of reviewing, in bug 461112. We have an unimplemented, never shown or used backend for a context menuitem to search for selected text (copied from Fx's copying from SM), that I couldn't quite bear to tear out when I rewrote nsContextMenu.js. Even though I saw that the patch in 461112 was removing the searchText string and switching to loading messenger.properties instead of contentAreaCommands.properties, I failed to notice that the patch wasn't actually adding searchText there.

But, in keeping with my claim that you shouldn't ever use strings which have been translated unused and unseen for years, the right thing to do is not to just add it back, and make localizers retranslate it while still not being able to see the result. Much as I hate to see it go, the right thing is to rip out the unused code until we actually use it.
Comment 1 Simon Paquet [:sipaq] 2008-11-04 04:56:03 PST
Thanks for patching my incomplete patch. 
And thanks for giving me the ability to show that we have lots of unused areas of code, by delivering an incomplete patch :)
Comment 2 Mark Banner (:standard8) 2008-11-04 06:44:46 PST
Comment on attachment 346224 [details] [diff] [review]
Fix v.1

So the patch reduces to:

>   isTextSelection : function CM_isTextSelection() {
>     var selection = this.searchSelected();
>+    if (selection != "")
>+      return true;
>+    return false;

how about:

return this.searchSelected();

much simpler.
Comment 3 Phil Ringnalda (:philor) 2008-11-04 10:34:47 PST
Comment on attachment 346224 [details] [diff] [review]
Fix v.1

Then I'll be forced to accept that there's no point in having a separate function, and no point in normalizing the whitespace, and before you know it I'll find myself fixing our share of bug 272041, too.
Comment 4 Phil Ringnalda (:philor) 2008-11-05 00:34:05 PST
Created attachment 346412 [details] [diff] [review]
Fix v.2

I guess actually fixing something is better than just preserving my favorite unused code.
Comment 5 Phil Ringnalda (:philor) 2008-11-05 22:26:56 PST
http://hg.mozilla.org/comm-central/rev/d8fa251d6de2
Comment 6 Nickolay_Ponomarev 2010-01-07 09:43:46 PST
For those who need the removed gContextMenu.isTextSelected (extensions ported from Firefox) here's the replacement:

var isTextSelected = false;
if (gContextMenu.isContentSelected) {
  var selection = document.commandDispatcher.focusedWindow.
      getSelection().toString().replace(/(\n|\r|\t)+/g, " ").trim();
  isTextSelected = (selection != "");
}

I'm linking this from 
https://developer.mozilla.org/en/XUL/PopupGuide/Extensions and https://developer.mozilla.org/En/Thunderbird_3_for_developers

Note You need to log in before you can comment on or make changes to this bug.