254542 - History window and history sidebar should accept middle-clicks

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Wladimir Palant]

Now that bug 72361 has been fixed, the same thing should be done for the history
window and the history sidebar. It should open items in tabs on middle clicks or
Ctrl-Enter and respect the browser.tabs.loadInBackground pref. History already
depends on bookmarks.js so there is no problem just reusing the bookmarks code.

Comment 1

[Wladimir Palant]

Attachment: Patch

Comment 2

[[e-mail]]

Comment on attachment 155341 [details] [diff] [review]
Patch

>         gPrefService = Components.classes["@mozilla.org/preferences-service;1"]
>                                  .getService(Components.interfaces.nsIPrefBranch);
>+        PREF = gPrefService;    // need this for bookmarks.js
You could consider renaming the uses of gPrefService to PREF, there aren't that
many.

> function historyOnClick(aEvent)
Hmm... these changes do weird things to ctrl+clicks...

>-          OpenURLArrayInTabs(URLArray);
>+          OpenURLArrayInTabs(URLArray, aTarget == "tab_background");
Couldn't you just pass aTarget?

>+                oncommand="OpenURL(BookmarksUtils.shouldLoadTabInBackground(event) ? 'tab_background' : 'tab');"/>
Hmm... getBrowserTargetFromEvent also has the same ?: so perhaps as a followup
the ?: should move into shouldLoadTabInBackground.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

>>         gPrefService = Components.classes["@mozilla.org/preferences-service;1"]
>>                               .getService(Components.interfaces.nsIPrefBranch);
>>+        PREF = gPrefService;    // need this for bookmarks.js
>
> You could consider renaming the uses of gPrefService to PREF, there aren't
> that many.

Please, no.  Using all caps for a global variable name is bad style.  Instead
fix bookmarks.js to use gPrefService (or gPrefs if it's important that the name
be short).

Comment 4

[[e-mail]]

Comment on attachment 155341 [details] [diff] [review]
Patch

OK, so I just noticed that this breaks history search. Possibly better to init
PREF in some way at the top of HistoryCommonInit and copy it into gPrefService.

Comment 5

[Wladimir Palant]

Attachment: Patch v2

Some sloppy testing on my part, sorry... This version of the patch now
initializes the pref service for any history window, including the search
results window. The sidebar history tree has been changed to single-select mode
like it is already the case with the bookmarks, this solves the problems with
ctrl+clicks (multiple selection wasn't really usable there anyway). And I fixed
historyOnClick(), the first patch was all wrong there.

I left the assignment |PREF = gPrefService| as it was. While I surely don't
want to change it in history.js (PREF is bad naming style), I can't change the
variable names in bookmarks.js either - Pike is saying that RDF is the
preferable variable name for the RDF service and shouldn't be changed. And IMO
it doesn't make anything better to rename only some of the variables.

As to replacing the boolean parameter/return value with aTarget - this saves
some characters but I think that boolean values are logically better here as
there really are only two possible values for aTarget, namely normal tabs or
tabs in background.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 155674 [details] [diff] [review]
Patch v2

sr=dmose.  sorry for the long delay.

Comment 7

[Wladimir Palant]

Attachment: Updated patch

Patch against a reasonably recent version, should be possible to apply now.
Taking the flags over from the previous version of the patch.

Comment 8

[[e-mail]]

Fix checked in.