545469 - folder preference for gloda indexing

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

To help some people manually manage gloda performance we should add a preference for indexing a folder to the folder properties dialog.

This preference can just be a simple checkbox which handles the current folder only.  For reference there is already a bug about a greater system ( bug 523649 ) which has inheritance for folder indexing properties.

I might rearrange some of the General properties items like the ( Rebuild Index ) button to be with this gloda preference and make them clearer that they are doing different operations.

Comment 1

[Kent James (:rkent)]

The inheritance is not any more difficult to do than a simple folder property, except for the user interface issues, as the separate inherit and setting done in GlodaQuilla is rather ugly. You also have the existing partial implementation in GlodaQuilla that you are welcome to copy.

Comment 2

[Kent James (:rkent)]

This needs to be resolved with Bug 523649. I think what you may say is that this is not a dup because 523649 want an inherited property but this does not, but in reality only one will be implemented so I think they should be combined and discussed together.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

How to present inheritance in the UI is indeed the defining problem.  It's not an intractable problem, but the cost/benefit is way too high to undertake at this time.

I think the right course of action at this time is to implement the checkbox and gloda logic without inheritance.

The main question is whether to use the same property ("glodaDoIndex") that GlodaQuilla uses even though the interpretations will be different.  Since GlodaQuilla can continue to override the logic used by gloda and clobber the non-inheriting checkbox UI out of existence, this mainly is a question of what should happen if glodaQuilla is uninstalled/disabled.

I would lean towards using the same property to minimize unpleasant surprises if the user runs Thunderbird in safe mode and so they can kinda-sorta keep their settings if they uninstall GlodaQuilla.  rkent, what are your thoughts on this?

Comment 4

[Kent James (:rkent)]

The number of users for GlodaQuilla is rather small, in the hundreds, and I have never requested nomination to public for it. So the problem of maintaining compatibility with existing GlodaQuilla users is not very large, and could probably be ignored.

But I still think many users will find a control of this at the single folder level quite frustrating. The inherited folder concept is quite powerful, allowing globallly disabling and selectively enabling in single folde trees, or application at the account level rather the folder level, all with a single property. I guess that I would hope to see a future revision of Thunderbird where most folder properties are made inherited for that reason, with a common user interface that the user comes to understand.

But I'm not willing to work on that UI at the moment, and it sounds like nobody else is either, so I guess I'll need to accept a future where in core you have a simpler per-folder feature, while an extension adds any support for inheritance. So what is the cleanest way to do that?

I think that separate names would be best. Otherwise adding and removing the extension would result in changes to which folder the option applies to, with very little easy way to control that. With separate names, when GlodaQuilla or the equivalent extension is removed, it would also remove the core override of the gloda indexing, so all of its changes would go away. If we shared names, then when GlodaQuilla is added or removed, then the folders that the property applies to would change with little way to really explain how that would happen.

I'm assuming here that if inherited properties are not done in core, then there will be still a subset of users who would prefer the perhaps uglier UI with the added flexibility of the inherited property.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #4)
> But I still think many users will find a control of this at the single folder
> level quite frustrating. The inherited folder concept is quite powerful,
> allowing globallly disabling and selectively enabling in single folde trees, or
> application at the account level rather the folder level, all with a single
> property.

I agree.  The inherited stuff is very powerful and what we're going to implement as part of this bug is comparably quite limited and potentially quite frustrating to use.

The simple property stuff will expose the functionality to the user in a straightforward fashion.  It should also provide the means for an add-on that wants to make things less frustrating for the user to do so while being consistent with our limited checkbox UI.

> But I'm not willing to work on that UI at the moment, and it sounds like nobody
> else is either, so I guess I'll need to accept a future where in core you have
> a simpler per-folder feature, while an extension adds any support for
> inheritance. So what is the cleanest way to do that?

I'm hoping an understandable and pleasant UI won't be too hard to accomplish in the future using jetpack and wmsy (a concise widget binding I toyed with at the end of last year and have not had time to return to yet).  But that obviously cannot happen in core for 3.1.

I will likely implement (or boss someone around to implement) the feature by providing a setter for indexingPriority on GlodaFolder (in datamodel.js).  Modifying the value will be permanently persisted to the folder (at least for disabling indexing) and also reflected in the gloda database.

Something that wanted to do inheritance and play within the system would poke that setting for all affected children folders when changing a parent folder, I suppose ignoring (and cutting the depth-traversal short) when it encounters a folder that has an explicit setting rather than just inheriting.  In order to properly reflect the inherited value in new folders, it would likely want to try and front-run the gloda indexer and set the indexing priority as a result of the folderAdded notification for the folder.  (Gloda does not listen on folderAdded, so this would be safe.  Note that the extension would probably want to use a setTimeout with 0 since the folderAdded notification is currently synchronous and it would probably be best to let the native code return first.)

I expect the gloda indexing priority would be stored in a "glodaIndexingPriority" attribute.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

I'm pretty sure we meant for this to be a 3.1 beta 2 blocker as this is one of those "people upgrading need an escape hatch" and it's easy to do but it has a string so it's now or never.

Assigning to myself for now, although I would not complain if someone else took it off my hands.

Comment 7

[Simon Paquet [:sipaq]]

This patch didn't make it for the string freeze. Adding late-l10n keyword and
informing Thunderbird localizers.

Developers should be aware of the process that we have for this, which is
detailed at
https://developer.mozilla.org/en/Thunderbird_Localization#Breaking_the_string_freeze

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: current folder properties dialog

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: possible alternative folder properties dialog

Here's what I've changed from the original.

I placed the ( Rebuild Index ) option in the bottom button area but I don't think I'm going to leave it there.  Likely I'll create a group box in the general area to help explain the Rebuild Index option as it's confusing with the Gloda Index options.

I moved the check folder for new messages out by removing the "indent" class as it's not related to the Default Character Encoding

I added the Gloda Index option at the bottom.  I'm calling it "[x] Include this folder in the Global Search index".  I looked at what the Mac and Windows use for text describing opt-out behavior of search indexing and this is fairly close to those.

So far all my changes are using the Dom inspector but I think I could at least get a UI patch done pretty quickly and leave the rest to someone else.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

I don't think this bug has notes on the other logic required for full happiness.  When we see the transition to "don't index this folder", we want to:

1) Kill any indexing jobs currently in the folder.  (We have logic for this related to compaction.)

2) Mark all already indexed gloda messages in the folder as deleted and likely kick off an indexing sweep to process the deleted messages.

The unit tests would likely then be:

a) Be indexing a folder and then mark it deleted and make sure the job terminates without completing indexing.

b) Have a folder with 1 already indexed message.  Mark the folder as don't index, have that message appear deleted.  Add another message, make sure it doesn't get indexed.

The two tests cases noted above may potentially be consolidated.  As noted before, compaction may be a good source of existing tests; the junk folder-related tests may also have something good.

Comment 11

[Ben Bucksch (:BenB)]

Bryan, have you considered putting "Rebuild index" and "gloda index" on the "Synchronization" tab? For me, they logically belong there, no?

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: current look-and-feel

OK, here's what I've got at the moment, based on your recent mockup and our discussions.  I've left "Global Search" capitalized to match your mockups (my personal feeling, FWIW, is that non-capitalized looks better because caps look out-of-place there).  

I've added a groupbox with a label with some explanatory text in it.  Some random notes:

* I'm not thrilled with "May fix behavioral issues", but it's a start.  I almost want to rename the button "Repair Folder Index", despite the fact that "Rebuild" is more strictly correct, because "Repair" communicates what the user is trying to do much more clearly, I think.

* I tried putting the explanatory text as the groupbox <caption>, but that looked sort of strange, which is why I switched to label.  I suspect some CSS loving could make the <caption> theory work better, though.

* There used to be a tooltip with explanatory text: "Rebuild Summary File".  This seemed useless and underinformative, so I've ditched it entirely in my current draft patch.
Thoughts?

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v1 (WIP)

Here's a work-in-progress patch (which, in fact, doesn't work yet :-).

Two things that I don't yet have a complete feel for, but which I suspect will become clearer as I continue to hack, are:

a) why it's worth maintaining two separate copies of the indexingPriority state (both in Gloda and the nsIMsgHeader

b) it appears to me that the logic for selecting the indexingPriority is set in datastore.js:_mapFolder().  This makes me suspect that if someone re-checks the checkbox after having it unchecked, simply setting it to kIndexingPriorityNever is going to be wrong in some cases, and that the better thing to do is to extract that logic out of _mapFolder to someplace where it's callable by the indexingPriority setter.  Or something.

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #13)
> a) why it's worth maintaining two separate copies of the indexingPriority state
> (both in Gloda and the nsIMsgHeader

I don't think I'd really have a problem if gloda fetched the property from the nsIMsgFolder as needed, but I don't really want to have do to the analytical legwork to think about what would happen in the cases where Thunderbird shuts down without committing the SQLite transaction but the nsIMsgFolder's state (and we'll assume panacea.dat) did hit the disk.  That would likely require some aggressive recovery logic.

Whereas if we just use the GlodaFolder indexing priority as authoritative until we have to rebuild the database, we generally get consistency without headaches.  (The user may have a deja vu moment when they need to re-toggle the setting.)
 
> b) it appears to me that the logic for selecting the indexingPriority is set in
> datastore.js:_mapFolder().  This makes me suspect that if someone re-checks the
> checkbox after having it unchecked, simply setting it to kIndexingPriorityNever
> is going to be wrong in some cases, and that the better thing to do is to
> extract that logic out of _mapFolder to someplace where it's callable by the
> indexingPriority setter.  Or something.

Yes, the _mapFolder logic setup is somewhat sketchy and the potentially weird dependency issues don't make it any easier.  Maybe the simplest thing is to add a method to GlodaDatastore to take an nsIMsgFolder and return the default priority, and add a "resetIndexingPriority" method to GlodaFolder?

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Er, yeah, I meant nsIMsgFolder.  But why maintain this property there at all?  Is it unreasonable to expect all consumers to simply to talk to Gloda when they want information about Gloda indexingPriority?

Sure, resetIndexingPriority sounds fine.

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

The only reason we write the property to nsIMsgFolder at all is so that if/when we blow the gloda database away we don't lose the user's desire to not index the folder.

I agree that all UI should directly talk to gloda in terms of the gloda API.


Separate question... is the checkbox widget going to collapse out of existence when gloda is disabled?  We probably don't want to mislead people into thinking it does anything when gloda is turned off.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Checkbox widget collapsing sounds like a good idea.  I've added it to my todo list.

Comment 18

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #17)
> Checkbox widget collapsing sounds like a good idea.  I've added it to my todo
> list.


Perhaps it should be displayed as off and disabled in that case?

Separate question: is there a bug asking for removal of the "Apply default charset to all messages in the folder" checkbox?

Comment 19

[Ben Bucksch (:BenB)]

> * I'm not thrilled with "May fix behavioral issues", but it's a start.

hah! Moms will love this: "I want this for my child, too!"

"Repair" sounds good. (I'd make the button "Rebuild..." and the explanation "Repair...")

> * I tried putting the explanatory text as the groupbox <caption>, but that

A <caption> really is a caption, heading, title. Other themes (Win32?) make this bold and in/above the groupbox line. <description> is correct for explanatory text.

> is the checkbox widget going to collapse out of existence
> when gloda is disabled?

Or rather .disable = true instead of .collapsed = true.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

My understanding is that the UX thinking around disabling has evolved since XUL was first created.  Specifically, leaving elements disabled but visible tends to be frustrating for users, particularly when there are no obvious visual cues about how to enable those elements (which would be true in this case).

Comment 21

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #11)
> Bryan, have you considered putting "Rebuild index" and "gloda index" on the
> "Synchronization" tab? For me, they logically belong there, no?

I'd really like to get rid of the Rebuild index button or move it to another tab but it didn't strike me that it was related to synchronization.  I feel like logically those items are related to synchronization but that's not how people actually perceive the indexing option.  Right now I feel like putting the Rebuild Index in a explanatory text box will help a bit.


(In reply to comment #12)
> I've left "Global Search" capitalized to match your mockups (my
> personal feeling, FWIW, is that non-capitalized looks better because caps look
> out-of-place there).  

It's what we did in the advanced prefs for "Global Search and Indexer"

> I've added a groupbox with a label with some explanatory text in it.  Some
> random notes:

Awesome

> * I'm not thrilled with "May fix behavioral issues", but it's a start.  

I'd like to see this text above the button and possibly have the button aligned to the right.  Will try to do in a mockup so you can see what I mean.

> I
> almost want to rename the button "Repair Folder Index", despite the fact that
> "Rebuild" is more strictly correct, because "Repair" communicates what the user
> is trying to do much more clearly, I think.

I like "Repair Folder", I want to actually drop the word "Index" so we aren't confusing this option with search indexing.  Perhaps in our explanatory text we could use the word "index" to keep some backwards compatibility with all the docs out there that reference the "Rebuild Index" button.

> * I tried putting the explanatory text as the groupbox <caption>, but that
> looked sort of strange, which is why I switched to label.  I suspect some CSS
> loving could make the <caption> theory work better, though.

The caption is usually used for a  title of the groupbox of similar items.  I'd leave it out since we only have one item.  Unless we want to say "Support" or something.

> * There used to be a tooltip with explanatory text: "Rebuild Summary File". 
> This seemed useless and underinformative, so I've ditched it entirely in my
> current draft patch.
> Thoughts?

That's where I might use the "Rebuild Summary File Index" so we keep the word index somewhere.  But in general yes, it's likely only been seen by a few souls in this world.


bug 496119 had some discussion on the "Check this folder for new messages" option, I'd like to fix that string while we're here.


(In reply to comment #20)
> My understanding is that the UX thinking around disabling has evolved since XUL
> was first created.  Specifically, leaving elements disabled but visible tends
> to be frustrating for users, particularly when there are no obvious visual cues
> about how to enable those elements (which would be true in this case).

Excellent!  The student has become the teacher, I may go now :)

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

http://skitch.com/dmose/dygys/lanikaidebug has an updated look-and-feel.  I changed button text, the explanatory text, and put back the tooltip but with the "Rebuild Summary File Index" you recommended.

I rearranged the groupbox like it sounded like you were talking about, except something's wrong with my spacer so the button is still flush with the left edge rather than the right edge.  But pretend it's over on the right side for the purposes of this question: is that picture what you were thinking of?

I have to run for other obligations now, and will be back in ~4 hours.

I haven't had time to look the "Check this folder for new messages" bug and ponder another string; I may get to that later.

Thoughts?

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Bryan, I spent some time peering at bug 496119, and I think the text suggested in comment 16 is pretty close to what's going on under the hood, although I don't believe autosync is really an issue here.  Flipping the order of the clauses in that sentence makes it seem clearer to me:

"When getting new messages for this account, always check this folder"

It still has the problem of being pretty long, and it's not obvious to me how to shorten it.  But I can happily switch to the above text or something similar if you wish.

FWIW, I believe what's actually going on under the hood and the _real_ (but significantly harder) fix is described in bug 496119 comment 24.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v2 (WIP)

Here's a new, incomplete WIP patch, with some gotchas.

Andrew, I'm bumping up against two issues here:

datastore.js, like datamodel.js, has no access to either GlodaIndexer or GlodaMsgIndexer.  Spending some time looking through the code, it wasn't obvious to me how make the datamodel or datastore dostuff with the indexer without violating encapsulation or adding hooks into the datamodel or datastore that the indexer specifically knows about and calls into.  Or something.  

So for the moment, I've simply commented out those bits, and I haven't implemented test b), on the theory that we'll eventually get consistency, and that's good enough.

Happily, however, test a) is now implemented and passes!

Secondly, we had talked about having the prefs UI use Gloda as its preferred API to call the setter on the datamodel.  However, Gloda doesn't actually appear to expose GlodaFolder as a public API.  I could, I suppose, use the nsIMsgFolder property and an nsIFolderListener that watches for it as the mechanism to communicate the changed pref from the prefs UI code to the datamodel, but that seems fairly baroque.

Thoughts on all this?

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v3 (WIP)

Last patch had a thinko; fixed and now the test actually passes.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #24)
> Secondly, we had talked about having the prefs UI use Gloda as its preferred
> API to call the setter on the datamodel.  However, Gloda doesn't actually
> appear to expose GlodaFolder as a public API.  I could, I suppose, use the
> nsIMsgFolder property and an nsIFolderListener that watches for it as the
> mechanism to communicate the changed pref from the prefs UI code to the
> datamodel, but that seems fairly baroque.

Gloda.getFolderForFolder(nsIMsgFolder)

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #24)
> datastore.js, like datamodel.js, has no access to either GlodaIndexer or
> GlodaMsgIndexer.  Spending some time looking through the code, it wasn't
> obvious to me how make the datamodel or datastore dostuff with the indexer
> without violating encapsulation or adding hooks into the datamodel or datastore
> that the indexer specifically knows about and calls into.  Or something.  

Yeah, this is the real gordian knot.

I think I pointed out public.js uses some proxying magic to make public some methods on the indexer/message indexer.

Maybe the right thing to do is to expose a public API on the message indexer that lets us explicitly change the priority of a folder?  That is probably most in line with how things happen with compaction.


Your idea about using a folder listener is a good one.  I think I was originally leaning that way, but changed my mind in favor of a more explicit API rather than having conventions on setting properties on folders.  Assuming you can make the former work, I would run with that?


I'll take a look at your patch and the tests now.

Comment 28

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 440985 [details] [diff] [review]
fix, v3 (WIP)

Looking very good thus far, but we need to address the other cases too.
http://reviews.visophyte.org/r/440985/

on file: mailnews/base/content/folderProps.xul line 88
>       <checkbox id="folderIncludeInGlobalSearch"
>                 label="&folderIncludeInGlobalSearch.label;"
>                 accesskey="&folderIncludeInGlobalSearch.accesskey;"/>
>       <checkbox hidefor="movemail,pop3,none,nntp"

we probably want a hidefor="nntp" here too since gloda won't index news ever


on file: mailnews/db/gloda/modules/datamodel.js line 229
>   this.indexingPriority = aIndexingPriority;

this change seems wrong


on file: mailnews/db/gloda/modules/datamodel.js line 355
>   set indexingPriority(aPriority) {
> 

We should probably have an early-bail case for when they are trying to set a
priority that is already active.


on file: mailnews/db/gloda/modules/datamodel.js line 357
>       this._datastore.updateFolderIndexingPriority(this);
>       this._indexingPriority = aPriority;

It seems like you should update the priority attribute before calling the
update function?


on file: mailnews/db/gloda/modules/datamodel.js line 374
>         // re-index
>         // XXX is this aggressive enough? 
>         // YYY indexingSweepNeeded instead?
>         //GlodaMsgIndexer.indexing = true;

You need to trigger an indexing sweep for deletion processing to occur, which
is what we want.


on file: mailnews/db/gloda/modules/datamodel.js line 378
>       }
>       

There should also be a case that detects we transitioned from 'never' to 'yes'
(which also needs to differentiate from 'yes, some priority' to 'yes, some
other priority') which should mark the folder filthy as it re-enables it.  And
then initiate the indexing sweep.


on file: mailnews/db/gloda/test/unit/base_index_messages.js line 261
> function test_indexing_never_priority() {

We should probably also verify that the change hit the database. 
sqlExpectCount in glodaTestHelper.js is probably the easiest way to do this.

Comment 29

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: updated text and switched to a grid inside the groupbox for improved layout

Thanks to dmose for talking me through this patch over IRC.  This looks good to me so I'll slap the ui-r+ on it right now.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v5 (WIP, but almost there!)

Comment 31

[Andrew Sutherland [:asuth] (he/him)]

We will likely want the following follow-up test but it doesn't have to land with this fix:

- Create a folder and assign it the persisted string property telling gloda to never index the folder.
- Call Gloda.getFolderForFolder on the folder and ensure that gloda decides the folder should never be indexed.

This will work because gloda does not actually listen for folder creation, so it is very easy to get the jump on gloda in this regard.  The key is not to put any messages in the folder, because gloda does get concerned about that.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v6

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 441250 [details] [diff] [review]
fix, v6

r=asuth with a few additional tweaks requested via IRC.

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: fix, v7 (done!)

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

Pushed: <http://hg.mozilla.org/comm-central/rev/a0d58747ab2e>.

Added in-testsuite? so that the automated test requested in comment 31 doesn't get forgotten.

Mail sent to dev-l10n regarding the late-l10n checkin.