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)
SeaMonkey
UI Design
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jwkbugzilla, Assigned: jwkbugzilla)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
9.93 KB,
patch
|
jwkbugzilla
:
review+
jwkbugzilla
:
superreview+
asa
:
approval1.8b3+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•20 years ago
|
||
Assignee: guifeatures → trev
Status: NEW → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Attachment #155341 -
Flags: superreview?(dmose)
Attachment #155341 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 2•20 years ago
|
||
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•20 years ago
|
||
>> 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•20 years ago
|
||
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-
Assignee | ||
Updated•20 years ago
|
Attachment #155341 -
Flags: superreview?(dmose)
Assignee | ||
Comment 5•20 years ago
|
||
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.
Assignee | ||
Updated•20 years ago
|
Attachment #155341 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #155674 -
Flags: review?(neil.parkwaycc.co.uk)
Updated•20 years ago
|
Attachment #155674 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Assignee | ||
Updated•20 years ago
|
Attachment #155674 -
Flags: superreview?(dmose)
Updated•20 years ago
|
Product: Core → Mozilla Application Suite
Comment 6•19 years ago
|
||
Comment on attachment 155674 [details] [diff] [review] Patch v2 sr=dmose. sorry for the long delay.
Attachment #155674 -
Flags: superreview?(dmose) → superreview+
Assignee | ||
Comment 7•19 years ago
|
||
Patch against a reasonably recent version, should be possible to apply now. Taking the flags over from the previous version of the patch.
Assignee | ||
Updated•19 years ago
|
Attachment #155674 -
Attachment is obsolete: true
Attachment #186290 -
Flags: superreview+
Attachment #186290 -
Flags: review+
Updated•19 years ago
|
Attachment #186290 -
Flags: approval1.8b3?
Updated•19 years ago
|
Attachment #186290 -
Flags: approval1.8b3? → approval1.8b3+
Comment 8•19 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•