bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Reimplement grouping in the main history window

RESOLVED FIXED in seamonkey2.0a3

Status

SeaMonkey
UI Design
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

({regression})

Trunk
seamonkey2.0a3
regression
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

10 years ago
As landed, the Places history window only supports a flat view.
(Assignee)

Comment 1

10 years ago
Created attachment 352293 [details] [diff] [review]
First revision

* Shared all sorts of stuff:
  o Initialisation
  o Script inclusion
  o View popup
  o Variables
  o Element IDs
  o Search (and removed PlacesSearchBox)
* Fixed the visit count display of a collapsed container
* Rewrote the ViewMenu "object" (but I don't see the point of an object...)
* Removed column header context menu
* Removed the flat list properties and related code
* Removed unused cached insertion point while I was there ;-)
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #352293 - Flags: ui-review?(kairo)
Attachment #352293 - Flags: superreview?(jag)
Attachment #352293 - Flags: review?(iann_bugzilla)

Comment 2

10 years ago
Comment on attachment 352293 [details] [diff] [review]
First revision

sr=jag

+    <toolbarbutton id="viewButton" type="menu" style="-moz-user-focus: normal;"
+            label="&view.label;">

Move |label| underneath |id|.

In places.js you left |_content: null,| it looks like that can be removed.

Re |var ViewMenu|, I guess it's used as a namespace. Up to you whether to keep it or not.

Does |sortingMode| get reset when you switch what you're sorting on, or will this code end up carrying over the sort direction from what you previously sorted on? Not sure we'd want that.
Attachment #352293 - Flags: superreview?(jag) → superreview+
(Assignee)

Comment 3

10 years ago
(In reply to comment #2)
> +    <toolbarbutton id="viewButton" type="menu" style="-moz-user-focus:
> normal;"
> +            label="&view.label;">
> Move |label| underneath |id|.
Bah, I get blamed when all I did was append a > ;-)

> In places.js you left |_content: null,| it looks like that can be removed.
Actually I'm angling on removing places.js ;-)

> Re |var ViewMenu|, I guess it's used as a namespace. Up to you whether to keep
> it or not.
I think I'd rather not actually.

> Does |sortingMode| get reset when you switch what you're sorting on, or will
> this code end up carrying over the sort direction from what you previously
> sorted on? Not sure we'd want that.
Yes, the old places code did that too; I didn't check what xpfe history did.
(Assignee)

Comment 4

10 years ago
(In reply to comment #3)
> (In reply to comment #2)
> > Does |sortingMode| get reset when you switch what you're sorting on, or will
> > this code end up carrying over the sort direction from what you previously
> > sorted on? Not sure we'd want that.
> Yes, the old places code did that too; I didn't check what xpfe history did.
Hmm, actually I'm not sure that the old places code did that after all, and the old xpfe history didn't, so I saved some more code by sharing that :-)
(Assignee)

Comment 5

10 years ago
Created attachment 352536 [details] [diff] [review]
Second revision

* Addressed jag's comments
* Eliminated setSort
* Eliminated the rest of places.js
* Eliminated the COLUMN_TYPE_ constants
* Included the locale and theme changes!
Attachment #352293 - Attachment is obsolete: true
Attachment #352536 - Flags: ui-review?(kairo)
Attachment #352536 - Flags: superreview?(jag)
Attachment #352536 - Flags: review?(iann_bugzilla)
Attachment #352293 - Flags: ui-review?(kairo)
Attachment #352293 - Flags: review?(iann_bugzilla)

Updated

10 years ago
Attachment #352536 - Flags: ui-review?(kairo) → ui-review+

Comment 6

10 years ago
Comment on attachment 352536 [details] [diff] [review]
Second revision

Basically the UI is OK, but I have some minor issues with this version still:

If you switch to no grouping, there's about an icon-width of free space left of the page icon, I guess this is where grouping has the twisties, is there an easy possibility to get rid of that space when we know we don't have twisties in the list because we have no grouping?

I see a rather large issue in the fact that we don't persist the grouping, both when closing the history window and opening again as well as when having the sidebar open and opening a new window (same on the sidebar when closing the app and reopening).

When testing, I just had the sidebar open, ungrouped, sorted by title by clicking on the column header, and grouped by date+site again. Now the date headers were sorted by their title, burying "Today" between "Older..." and "Yesterday". This looks quite strange. :-/

The old context menu for the column header was more useful than the "new" one (Copy/Delete). IMHO, we probably should either have the colpicker popup there generally, or the "view" popup in this case.

I also encountered some strangeness with sorting by title, where some titles apparently starting with random (?) alphabetic characters are sorted up front, before numbers and the correctly sorted rest of the entries with alphabetic characters, but that's probably something unrelated to that patch.
(Assignee)

Comment 7

10 years ago
(In reply to comment #6)
> If you switch to no grouping, there's about an icon-width of free space left of
> the page icon, I guess this is where grouping has the twisties, is there an
> easy possibility to get rid of that space when we know we don't have twisties
> in the list because we have no grouping?
That should be possible, we do something similar for unthreaded mail views.

> I see a rather large issue in the fact that we don't persist the grouping
D'oh! I went to all the trouble of reading our existing pref, only to forget to write it again... fortunately it's a 1-line fix.

> When testing, I just had the sidebar open, ungrouped, sorted by title by
> clicking on the column header, and grouped by date+site again. Now the date
> headers were sorted by their title, burying "Today" between "Older..." and
> "Yesterday". This looks quite strange. :-/
Actually I'm pretty sure the old history window had the same bug.

> The old context menu for the column header was more useful than the "new" one
> (Copy/Delete). IMHO, we probably should either have the colpicker popup there
> generally, or the "view" popup in this case.
OK, I'll look into this.

Comment 8

10 years ago
>> When testing, I just had the sidebar open, ungrouped, sorted by title by
>> clicking on the column header, and grouped by date+site again. Now the date
>> headers were sorted by their title, burying "Today" between "Older..." and
>> "Yesterday". This looks quite strange. :-/
> Actually I'm pretty sure the old history window had the same bug.
I can confirm that this happens in the old history window (Tested in 1.5a)

Phil

Comment 9

10 years ago
Comment on attachment 352536 [details] [diff] [review]
Second revision

Oh yeah, I meant to point out last time you could get rid of the anonid attributes. Nice catch.

historyOnSelect() updates gLastHostname and gLastDomain, but they no longer seem to exist. I was going to point out that the mechanism to determine the domain name won't do the right thing for e.g. "news.bbc.co.uk" (and I know we have code somewhere to handle that), but just drop that bit.


>diff -r 6d985db2ad97 suite/common/history/history.js
>--- a/suite/common/history/history.js  Wed Dec 10 12:12:00 2008 -0800
>+++ b/suite/common/history/history.js  Thu Dec 11 14:31:33 2008 +0000

>+function UpdateViewSort(aMenuItem) {
>+  // Note: consider building this by reading the result's sortingMode instead.
>+  var unsorted = true;
>+  var ascending = true;
>+  while (aMenuItem) {
>+    switch (aMenuItem.id) {
>+      case "": // separator
>+        break;
>+      case "Unsorted":
>+        if (unsorted) // this would work even if Unsorted was last
>+          aMenuItem.setAttribute("checked", "true");

If Unsorted is the first item, won't this check it, even when stuff is sorted?


>diff -r 6d985db2ad97 suite/common/history/utils.js
>--- a/suite/common/history/utils.js  Wed Dec 10 12:12:00 2008 -0800
>+++ b/suite/common/history/utils.js  Thu Dec 11 14:31:33 2008 +0000
>@@ -371,7 +311,13 @@

>  openNodeWithEvent: function PU_openNodeWithEvent(aNode, aEvent) {
>    var where = whereToOpenLink(aEvent);
>    if (aNode && where) {
>      if (PlacesUtils.nodeIsURI(aNode))
>        this.openNodeIn(aNode, where);
>      else if (PlacesUtils.nodeIsContainer(aNode) && where != "current")
>        this.openContainerNodeInTabs(aNode, aEvent);
>    }
>  },

...

>  openNodeIn: function PU_openNodeIn(aNode, aWhere) {
>    if (aNode && PlacesUtils.nodeIsURI(aNode) && this.checkURLSecurity(aNode)) {
>      this.markPageAsTyped(aNode.uri);
>      openUILinkIn(aNode.uri, aWhere);
>    } else if (aNode && PlacesUtils.nodeIsContainer(aNode) && aWhere != "current") {
>      this.openContainerNodeInTabs(aNode, aWhere);
>    }
>  },
>

You can rewrite those two as:

  openNodeWithEvent: function PU_openNodeWithEvent(aNode, aEvent) {
    this.openNodeIn(aNode, whereToOpenLink(aEvent));
  },

  openNodeIn: function PU_openNodeIn(aNode, aWhere) {
    if (!aNode || !aWhere) // or something like this
      return;

    if (PlacesUtils.nodeIsURI(aNode)) {
      if (this.checkURLSecurity(aNode)) {
        this.markPageAsTyped(aNode.uri);
        openUILinkIn(aNode.uri, aWhere);
      }
    } else if (PlacesUtils.nodeIsContainer(aNode) && aWhere != "current") {
      this.openContainerNodeInTabs(aNode, aWhere);
    }
  },

What should happen, btw, if in that last case |aWhere| is "current"? Should that never occur barring programmer error?
(Assignee)

Comment 10

10 years ago
(In reply to comment #9)
>historyOnSelect() updates gLastHostname and gLastDomain
Whoops, that's part of the forthcoming "clean up the context menu" patch ;-)

>>+      case "Unsorted":
>>+        if (unsorted) // this would work even if Unsorted was last
>>+          aMenuItem.setAttribute("checked", "true");
>If Unsorted is the first item, won't this check it, even when stuff is sorted?
Sure, but radio menuitems are smart enough so that only the last checked item sticks. I could have removed the test; that would rely on Unsorted being first.

>You can rewrite those two as:
Hmm, I thought there were other callers, I'll check.

>What should happen, btw, if in that last case |aWhere| is "current"? Should
>that never occur barring programmer error?
Oh, programmer error means that it will almost always occur ;-) Seriously I might have subsequently rewritten the caller not to call it in that case.

Comment 11

10 years ago
Comment on attachment 352536 [details] [diff] [review]
Second revision

On code inspection to start with:
>diff -r 6d985db2ad97 suite/common/history/history.js

>+function HistoryCommonInit()
> {
Bracing in this file seems to be back and forth but most seem to be on same line, so brace on same line as function.

>+  try {
>+    gHistoryGrouping = gPrefService.getCharPref("browser.history.grouping");
>+  } catch (e) {
>+  }
Inconsistent bracing, further down you have } catch (e) {}

>+function ToggleColumn(aMenuItem) {
Could this be called with the id rather than the menuitem?
>+  var colid = aMenuItem.id.replace(/Toggle/, "");
>+  var column = document.getElementById(colid);
>+  column.setAttribute("hidden", !column.hidden);
>+}
>+
>+function GroupBy(aMenuItem)
> {
Nit: Bracing
Again could this be called with the id rather than the menuitem?
>+  gHistoryGrouping = aMenuItem.id.replace(/GroupBy/, "").toLowerCase();
>   gSearchBox.value = "";
>   searchHistory("");
> }

>diff -r 6d985db2ad97 suite/common/history/placesOverlay.xul

>+  <menupopup id="viewPopup">
>+    <menu id="viewColumns"
>+          label="&view.columns.label;" accesskey="&view.columns.accesskey;">
>+      <menupopup onpopupshowing="UpdateViewColumns(this.firstChild);"
>+                 oncommand="ToggleColumn(event.target);">
event.target.id?

>+    <menu id="viewGroupBy" label="&view.groupBy.label;"
>+          accesskey="&view.groupBy.accesskey;">
>+      <menupopup oncommand="GroupBy(event.target);">
event.target.id?

Comment 12

10 years ago
Comment on attachment 352536 [details] [diff] [review]
Second revision

On testing, I've not noticed anything that has not already been pointed out by Kairo/jag.
Attachment #352536 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 13

10 years ago
(In reply to comment #11)
>(From update of attachment 352536 [details] [diff] [review])
>>+function HistoryCommonInit()
>> {
>Bracing in this file seems to be back and forth
My fault, this file is supposed to have them on a new line.

>>+  try {
>>+    gHistoryGrouping = gPrefService.getCharPref("browser.history.grouping");
>>+  } catch (e) {
>>+  }
>Inconsistent bracing, further down you have } catch (e) {}
Fixed.

>>+      <menupopup onpopupshowing="UpdateViewColumns(this.firstChild);"
>>+                 oncommand="ToggleColumn(event.target);">
>event.target.id?
Actually I wanted to use event.target for SortBy but XPConnect wouldn't let me.
(Assignee)

Comment 14

10 years ago
(In reply to comment #9)
> What should happen, btw, if in that last case |aWhere| is "current"? Should
> that never occur barring programmer error?
No, that happens if you select a group (day/site) and hit Enter, for instance.
(Assignee)

Comment 15

10 years ago
Created attachment 352929 [details] [diff] [review]
Final patch

OK, this one should have all those loose ends tied up.
Attachment #352536 - Attachment is obsolete: true
Attachment #352929 - Flags: superreview?(jag)
Attachment #352536 - Flags: superreview?(jag)

Comment 16

10 years ago
In treeView.js:

+  isEditable: function(aRow, aColumn) { return false; },
+  setCellText: function(aRow, aColumn, aText) { },
   selectionChanged: function() { },
+  cycleCell: function(aRow, aColumn) { },

Part of another patch?


In history/utils.js:

+      dump(key + "\n" + e + "\n");
(Assignee)

Comment 17

10 years ago
(In reply to comment #16)
>>+  isEditable: function(aRow, aColumn) { return false; },
>>+  setCellText: function(aRow, aColumn, aText) { },
>>   selectionChanged: function() { },
>>+  cycleCell: function(aRow, aColumn) { },
>Part of another patch?
No, but I'd removed the old definitions (they were only useful for bookmarks).

>>+      dump(key + "\n" + e + "\n");
D'oh. That whole hunk is for debugging only.
(Assignee)

Comment 18

10 years ago
Created attachment 353129 [details] [diff] [review]
Extra cleanup

As well as removing that spurious dump I also removed support for the show sessions option which is not exposed in the Firefox UI either.
Attachment #352929 - Attachment is obsolete: true
Attachment #353129 - Flags: superreview?(jag)
Attachment #352929 - Flags: superreview?(jag)

Updated

10 years ago
Attachment #353129 - Flags: superreview?(jag) → superreview+
(Assignee)

Comment 19

10 years ago
Pushed changeset bab0bf9760fa to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0a3

Updated

10 years ago
Blocks: 470195
Depends on: 470544
(Assignee)

Comment 20

10 years ago
Created attachment 358024 [details] [diff] [review]
Followup tweak

While reviewing the dlmgrui patch I noticed that the in old History window the column headers cycled between sorted, reversed and unsorted. This restores that.
Attachment #358024 - Flags: review?(iann_bugzilla)

Updated

10 years ago
Attachment #358024 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 21

10 years ago
Pushed changeset fa5d23ec1600 to comm-central.
You need to log in before you can comment on or make changes to this bug.