313331 - autocomplete file names in xul filepicker

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[[e-mail]]

Loosely based on the W2K/XP filepicker, but only completing the current folder.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Obviously I'll also consider comments from anyone else prepared to look ;-)

Comment 2

[[e-mail]]

Attachment: It helps if I attach the correct patch ;-)

Comment 3

[Benjamin Smedberg]

I will get to this Friday or next Monday.

Comment 4

[Benjamin Smedberg]

Comment on attachment 200403 [details] [diff] [review]
It helps if I attach the correct patch ;-)

+ifndef MOZ_PHOENIX
+ifndef MOZ_XULRUNNER

Please replace this with

ifneq (1_,$(MOZ_XUL_APP)_$(MOZ_THUNDERBIRD))

+  nsCOMPtr<nsISupportsArray> resultItems;
+  results->GetItems(getter_AddRefs(resultItems));

You use this later without either null-checking or rv-checking.

Comment 5

[[e-mail]]

Attachment: Better patch

Also includes the fixes to toolkit's mysteriously forked files :-P

Comment 6

[jag (Peter Annema)]

Comment on attachment 216583 [details] [diff] [review]
Better patch

>Index: xpfe/components/autocomplete/resources/content/autocomplete.xml
>===================================================================

General comment: wow, this file could use some empty lines to demarcate blocks, functions in prototype definitions, etc. I understand you're just keeping in style, but if you feel some urge to go fix this, sr=jag on that any time! :-)

>+      <property name="searchSessions" onget="return this.getAttribute('searchSessions')"/>

No |readonly="true"|?


>+      <method name="initautocompletesearch">

interCaps?


>Index: xpfe/components/filepicker/src/nsFileView.cpp
>===================================================================

>+nsFileResult::nsFileResult(const nsAString& aSearchString,
>+                           const nsAString& aSearchParam):
>+    if (mSlashPos != kNotFound)
>+      NS_NewLocalFile(Substring(mSearchString, 0, mSlashPos + 1), PR_FALSE, getter_AddRefs(directory));

Why not follow symlinks?


>+      if (mSlashPos > 0)
>+        directory->AppendRelativePath(Substring(mSearchString, 0, mSlashPos));
...
>+    while (NS_SUCCEEDED(dirEntries->HasMoreElements(&hasMore)) && hasMore) {
...
>+      if (StringBeginsWith(fileName, Substring(mSearchString, mSlashPos + 1))) {
>+        fileName.Insert(Substring(mSearchString, 0, mSlashPos + 1), 0);

Move these two |Substring|s out of the loop. If that |AppendRelativePath()| above is ok with appending something ending in a '/' you could even create these two real early and re-use (otherwise it'd be just the one).


>+NS_IMETHODIMP nsFileResult::GetSearchString(nsAString & aSearchString)
>+{
>+  aSearchString.Assign(mSearchString);

aSearchString = mSearchString; ?


>+class nsFileComplete : public nsIAutoCompleteSearch
>+{
>+public:
>+  nsFileComplete() {};

This constructor is superfluous.


>@@ -190,6 +344,11 @@ nsFileView::SetShowOnlyDirectories(PRBoo
>   } else {
>     // Run the filter again to get the file list back
>     FilterFiles();
>+
>+    SortArray(mFilteredFiles);
>+    if (mReverseSort)
>+      ReverseArray(mFilteredFiles);

Hmmm, this is border-line worth turning into a function, or maybe have |SortInternal(PRBool aSortDirs = PR_TRUE)|. Or something.

sr=jag on the xpfe bits.

Comment 7

[[e-mail]]

Comment on attachment 216583 [details] [diff] [review]
Better patch

Maybe bz would appreciate this more :-)

Comment 8

[Boris Zbarsky [:bzbarsky]]

So when do we actually use this nowadays?  We use the GTK2 POS on anything resembling a modern Linux system for our default GTK2 builds, no?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 216583 [details] [diff] [review]
Better patch

Note that I don't know much about our autocomplete APIs, so it'll take me a bit to review this...

Comment 10

[[e-mail]]

(In reply to comment #8)
>So when do we actually use this nowadays?  We use the GTK2 POS on anything
>resembling a modern Linux system for our default GTK2 builds, no?
Oh right, I forgot you had a modern Linux system these days ;-) You could --enable-default-toolkit=gtk or you could hack compreg.dat or widget/src/gtk2/nsWidgetFactory.cpp or you could land the pref patch or ...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 216583 [details] [diff] [review]
Better patch

Filepicker js/xul changes look fine to me.  ;)

As for the autocomplete changes, I'm mostly doing very general review:

>Index: xpfe/components/autocomplete/resources/content/autocomplete.xml

>+      <field name="mAutoCompleteSession"><![CDATA[

This holds a ref to |this| in a closure, right?  Don't we need to clean this up in our destructor to avoid leaking?

>+          QueryElementAt: function(aIndex, aIID) {

Is it ok that this returns non-null even if the retval doesn't QI to aIID?

>Index: xpfe/components/filepicker/src/nsFileView.cpp

>+nsFileResult::nsFileResult(const nsAString& aSearchString,
>+  mSearchResult(RESULT_FAILURE)

Why bother if the body inits it anyway?

Also, I'd like to see some documentation of what the args to this constructor mean.

>+      NS_NewLocalFile(Substring(mSearchString, 0, mSlashPos + 1), PR_FALSE, getter_AddRefs(directory));

You use that substring a good bit.  Why not safe a ref to it on the stack, with a useful name too?

Rest looks ok to me.

Comment 12

[[e-mail]]

Fix checked in on the trunk.

(In reply to comment #11)
>>+      <field name="mAutoCompleteSession"><![CDATA[
>This holds a ref to |this| in a closure, right?  Don't we need to clean this up
>in our destructor to avoid leaking?
I copied that from somewhere else so if it does leak it was already leaking...

>>+          QueryElementAt: function(aIndex, aIID) {
>Is it ok that this returns non-null even if the retval doesn't QI to aIID?
Yes, only autocomplete.xml calls this.

>>+nsFileResult::nsFileResult(const nsAString& aSearchString,
>>+  mSearchResult(RESULT_FAILURE)
>Why bother if the body inits it anyway?
Because on my first draft it didn't :-)

>Also, I'd like to see some documentation of what the args to this constructor mean.
Done.

>>+      NS_NewLocalFile(Substring(mSearchString, 0, mSlashPos + 1), PR_FALSE, getter_AddRefs(directory));
>You use that substring a good bit.  Why not safe a ref to it on the stack, with a useful name too?
Which is basically what jag said, but I missed this case.

Comment 13

[[e-mail]]

Attachment: polish

* changed slash pos from a member into a local
* tweaked the absolute path detection code

Comment 14

[[e-mail]]

Fixed on the branch, eventually...

Comment 15

[[e-mail]]

Polish checked in to the trunk.