473007 - Reimplement delete history for site or domain in the main history window

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

As landed, the Places history window lacks the delete for site or domain menus.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Vlado Valastiak [:wladow]]

Comment on attachment 356346 [details] [diff] [review]
Proposed patch

>+delete.hostname.true=Delete History for %S
>+delete.hostname.false=Delete History for Site
>+delete.hostname.accesskey=S
>+delete.domain.true=Delete History for *.%S
>+delete.domain.false=Delete History for Domain
>+delete.domain.accesskey=H
>

Could you add a localization note here saying the .accesskey is used for both .true and .false strings?

Comment 3

[Ian Neal]

Comment on attachment 356346 [details] [diff] [review]
Proposed patch

delete.hostname.accesskey=e is a better choice I think
Select All no longer works with this patch applied, though Ctrl-A does.
The Edit menu doesn't seem to do the right thing in a number of cases:
* When you have more than one thing selected, the Edit menu still has the last single selection in the "Delete History..." entries.
* When grouping by Day and Site, selecting Site does not give the correct entries in the Edit menu.
* Selecting a Day and then looking at the Edit Menu again does not have the correct entries.

Comment 4

[[e-mail]]

Attachment: Updated for review comments

* Changed the accesskey from S to s so that it doesn't move
* Fixed the select all menuitem
* Changed from the algorithm copied from the old xpfe history manager to a new system that works with grouping containers and clears out stale entries.

Comment 5

[Ian Neal]

Comment on attachment 357591 [details] [diff] [review]
Updated for review comments

>+    if (gLastHostname) {
>+      var match = gLastHostname.match(/([^.]+\.[^.]+$)/);
>+      if (match)
>+        gLastDomain = match[1];
>+    }
Almost there, still an issue with sites that have more than 3 parts to their hostname:
www.acme.com gives *.acme.com whereas www.acme.co.uk gives *.co.uk
The correction to this should make sure it still does the right thing for 2 part hostnames.
When looking at deleting history my first instinct was to bring up a context menu which, at the moment, does not give the site/domain delete options. It would be nice if it did, if it is a simple thing to add.

Comment 6

[[e-mail]]

Attachment: Using ETLD service

Attachment 357591 [details] [diff] just copied the domain code from the old history window.
This version uses the ETLD service instead.

I still want to do some context menu cleanup in another bug (not filed yet) so I'd rather include them then (remind me if I forget!)

Comment 7

[[e-mail]]

Attachment: Really using ETLD service

Comment 8

[[e-mail]]

Pushed changeset 0a1bfdd0efa6 to comm-central.

Comment 9

[[e-mail]]

Attachment: Fix console spew

I hadn't realised that tree.xml calls updateHistoryCommands via the oncommandupdate attribute. While this means that we don't need to manually call it in HistoryCommonInit it does mean that we need to only update those commands that actually exist, so this patch fixes updateHistoryCommands to do that by moving the optional commands into the placesCommands commandset.

Comment 10

[Ian Neal]

Comment on attachment 361193 [details] [diff] [review]
Fix console spew

With this patch in place, changing the "Group By" in the history window (but not the history sidebar) gives the following "spew" in the terminal window:
An error occurred updating the cmd_selectAll command

Comment 11

[Ian Neal]

Probably completely unrelated, when you close a browser window with the history sidebar showing you get the following message in the error console:
Error: window.top.XULBrowserWindow is null
Source File: chrome://communicator/content/history/sidebarUtils.js
Line: 121

Comment 12

[[e-mail]]

Attachment: Fix more console spew

As attachment 361193 [details] [diff] [review], but additionally:
* Adds containerOpen check to prevent exception accessing childCount
* Removes completely bogus semicolon
* Removes bogus duplicate case values (should have been in bug 477150)
* Disables shift key (for parity with old history window)
* Checks that browser window is not closing before clearing its status

Comment 13

[[e-mail]]

Comment on attachment 361736 [details] [diff] [review]
Fix more console spew

Pushed changeset e17086610013 to comm-central.