[faceted search]: clicking on the "None" value of a facet does nothing

RESOLVED FIXED in Thunderbird 3.0rc1

Status

Thunderbird
Search
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: davida, Assigned: asuth)

Tracking

Trunk
Thunderbird 3.0rc1
Bug Flags:
blocking-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [no l10n impact][ready to land])

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
I'm not sure if the facet value should not look clickable, or if it should do something when we click it.
Flags: blocking-thunderbird3?
marking blocking and targeting to rc1.  If we can make it work by b4 that's great.

I think it'd be good to also use some more specific language in the future, e.g. "No Tags"
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0rc1
Duplicate of this bug: 516058
(Assignee)

Comment 3

8 years ago
It should do things when you click it.  It used to do so.  Constraint support re-write must have broke it.
Component: Mail Window Front End → Search
QA Contact: front-end → search

Updated

8 years ago
Whiteboard: [no l10n impact]
(Assignee)

Updated

8 years ago
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
(Reporter)

Comment 4

8 years ago
console sez:

Error: groupValue is null
Source File: chrome://messenger/content/glodaFacetView.js
Line: 234
(Assignee)

Comment 5

8 years ago
I have a mostly throw-away patch that tried to treat the None value as a completely special case that did not play well with the other values and was impossible to use as the basis for a gloda query.  During the course of creating this patch I was concluded the following strategy is instead what we want:

The None value is a sufficiently interesting case that attributes may want to be able to query on it.  However, it is also special enough that it should not be treated as one of the other 'in-domain' values and we certainly do not want to make it the responsibility of the attribute to deal with it.

For example, in the tag case, if a message has no tags we don't want the tag provider to have to create a synthetic null tag.  Instead, the attribute definition indicates that the empty list is significant.  So message.tags == [] instead of message.tags == ["null-sentinel-value"].  However, when indexing Gloda will emit a row into the database with a sentinel value so that query.tags(null) returns that message as a result.  It also becomes possible to have the faceting UI show only messages tagged "awesome" or that are not tagged.  (query.tags("awesome", null).)
Whiteboard: [no l10n impact] → [no l10n impact][has in-progress patch, fix strategy]
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][has in-progress patch, fix strategy] → [no l10n impact][has in-progress patch, fix strategy][will work this on Nov 8]
(Assignee)

Comment 6

8 years ago
Created attachment 411152 [details] [diff] [review]
v1 special empty value support

This was totally the right thing to do.  The semantics work wonderfully and the actual changes to the faceting code were almost non-existent.

The bad news is that I am revving the schema version again, so kiss your gloda databases goodbye once more.

There is some noise in the patch:

- To make the unit test more thorough I needed to make various code supporting Widget in test_query_core.js a bit more robust and a bit more ugly.  (Gloda generally assumes immutable things, so I had to jump through hoops to make the mutable data structure look immutable.)

- logException has had a bug for a while where it prints the stack trace at the time you call it rather than the stack trace of the exception.
Attachment #411152 - Flags: review?(bienvenu)
(Assignee)

Updated

8 years ago
Whiteboard: [no l10n impact][has in-progress patch, fix strategy][will work this on Nov 8] → [no l10n impact][has patch, needs review bienvenu]

Comment 7

8 years ago
Comment on attachment 411152 [details] [diff] [review]
v1 special empty value support

I read through this and it all looks OK. I need to run with the patch and make sure it fixes the bug, and that tests works, which I'll do in a few minutes.

incomplete comment:
+   * @param [aAttrDef.emptySetIsSignificant=false] Should we
    * @param aAttrDef.subjectNouns A list of object types (NOUNs) that this

Comment 8

8 years ago
Comment on attachment 411152 [details] [diff] [review]
v1 special empty value support

this works better, but it's not fully working. I've seen several instances of clicking None in the tagged category, and saying "must not be tagged" and the search results don't change. I think I saw the same thing with mailing lists. It's not every time, however.

I do see some console warnings from jquery, but they seem to be static warnings.

The xpcshell tests all pass.

Comment 9

8 years ago
Comment on attachment 411152 [details] [diff] [review]
v1 special empty value support

I can't reproduce the issue I thought I saw - perhaps I was confused about what counts I was expecting to see after adding a none constraint.
Attachment #411152 - Flags: review?(bienvenu) → review+
Whiteboard: [no l10n impact][has patch, needs review bienvenu] → [no l10n impact][ready to land]
(Assignee)

Comment 10

8 years ago
(In reply to comment #9)
> (From update of attachment 411152 [details] [diff] [review])
> I can't reproduce the issue I thought I saw - perhaps I was confused about what
> counts I was expecting to see after adding a none constraint.

We have some bugs related to deleted tags I think, definitely if the deletion happens during the same session.  Although I doubt that your testing includes deleting tags, it's a possible explanation.
(Assignee)

Comment 11

8 years ago
pushed to trunk:
http://hg.mozilla.org/comm-central/rev/f3a99b4c8275

pushed to comm-1.9.1:
http://hg.mozilla.org/releases/comm-1.9.1/rev/d8c84deb69ad
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.