Closed Bug 254542 Opened 20 years ago Closed 19 years ago

History window and history sidebar should accept middle-clicks

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jwkbugzilla, Assigned: jwkbugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Patch (obsolete) — Splinter Review
Assignee: guifeatures → trev
Status: NEW → ASSIGNED
Attachment #155341 - Flags: superreview?(dmose)
Attachment #155341 - Flags: review?(neil.parkwaycc.co.uk)
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.
>>         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 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.
Attachment #155341 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attachment #155341 - Flags: superreview?(dmose)
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #155341 - Attachment is obsolete: true
Attachment #155674 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #155674 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #155674 - Flags: superreview?(dmose)
Product: Core → Mozilla Application Suite
Comment on attachment 155674 [details] [diff] [review]
Patch v2

sr=dmose.  sorry for the long delay.
Attachment #155674 - Flags: superreview?(dmose) → superreview+
Attached patch Updated patchSplinter Review
Patch against a reasonably recent version, should be possible to apply now.
Taking the flags over from the previous version of the patch.
Attachment #155674 - Attachment is obsolete: true
Attachment #186290 - Flags: superreview+
Attachment #186290 - Flags: review+
Attachment #186290 - Flags: approval1.8b3?
Attachment #186290 - Flags: approval1.8b3? → approval1.8b3+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: