Convert search dialog folderpicker to newer binding, it's prettier -- and rdf free.

RESOLVED FIXED in Thunderbird 24.0

Status

Thunderbird
Search
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

(Blocks: 2 bugs)

unspecified
Thunderbird 24.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

10.93 KB, patch
alta88
: review+
alta88
: ui-review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 757158 [details] [diff] [review]
patch


Seems like that picker was forgotten along the way..
Assignee: nobody → alta88
Attachment #757158 - Flags: ui-review?(richard.marti)
Attachment #757158 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 2

4 years ago
Created attachment 757179 [details] [diff] [review]
patch


fix SearchDialog.js - get rid of folder to URI to folder to URI hackiness, remove an unused function, ensure the new folderpicker icon is correct for css.  the Filter dialog picker should be likewise fixed.
Attachment #757158 - Attachment is obsolete: true
Attachment #757158 - Flags: ui-review?(richard.marti)
Attachment #757158 - Flags: review?(mkmelin+mozilla)
Attachment #757179 - Flags: ui-review?(richard.marti)
Attachment #757179 - Flags: review?(mkmelin+mozilla)

Comment 3

4 years ago
Nice.
But not sure what you mean with the Filter dialog. That one was already converted.
(Assignee)

Comment 4

4 years ago
I just meant the part about the icon missing and causing the box to be shrunken; the menulist just needs a class="folderMenuItem" and making sure _setCssSelectors is in there.  And the Run Filters picker shouldn't show/select the account, as it doesn't apply (bug 264384).  Unless of course, Run Filters gets a Subfolders option (like search).

Comment 5

4 years ago
Thanks, didn't notice the folderMenuItem class is also used on the menulists. I was mostly cleaning up the class="menulist-menupopup" on the menupopup child. So if you want to cleanup the icons on the menulist, then also the New Folder dialog is missing it. Or maybe they are intentionally not there in some places. Anyway, file the bug please (whether you intend to work on it or not) and CC me there.
Status: NEW → ASSIGNED
Comment on attachment 757179 [details] [diff] [review]
patch

This looks good. This makes the folder picker on main toolbar (#locationFolders), the two menulists in filter dialog, the picker in account manager and now in search dialog consistent without a tree.

The only difference I saw was the folder name instead the "choose this folder" menuitem, but I saw this is addressed in bug 878805.
Attachment #757179 - Flags: ui-review?(richard.marti) → ui-review+
(Assignee)

Comment 7

4 years ago
Created attachment 757435 [details] [diff] [review]
patch


this update just adds the IsFeedFolder="true" attribute to menulists using _setCssProperties.  once this is in, and bug 878805 is verified, the folderMenus.css can be updated to show both subs/no subs feed folder icons correctly.
Attachment #757179 - Attachment is obsolete: true
Attachment #757179 - Flags: review?(mkmelin+mozilla)
Attachment #757435 - Flags: ui-review+
Attachment #757435 - Flags: review?(mkmelin+mozilla)

Comment 8

4 years ago
(In reply to alta88 from comment #7)
> this update just adds the IsFeedFolder="true" attribute to menulists using
> _setCssProperties.  once this is in, and bug 878805 is verified, the
> folderMenus.css can be updated to show both subs/no subs feed folder icons
> correctly.
I don't think bug 878805 will add any new css. So if you need some, please add it.

(In reply to Richard Marti [:Paenglab] from comment #6)
> The only difference I saw was the folder name instead the "choose this
> folder" menuitem, but I saw this is addressed in bug 878805.
Actually bug 878805 proposes to make this change ("file here" -> folder name) also in all the other pickers. So depends on which direction you want this to be addressed.
(In reply to :aceman from comment #8)
> Actually bug 878805 proposes to make this change ("file here" -> folder
> name) also in all the other pickers. So depends on which direction you want
> this to be addressed.

I showed only the difference. The direction "file here" -> folder name would for me make sense. At the beginning of my TB usage I asked me sometimes, which folder is "choose this folder". This change could help not so experienced users.
(Assignee)

Comment 10

4 years ago
(In reply to :aceman from comment #8)
> (In reply to alta88 from comment #7)
> > this update just adds the IsFeedFolder="true" attribute to menulists using
> > _setCssProperties.  once this is in, and bug 878805 is verified, the
> > folderMenus.css can be updated to show both subs/no subs feed folder icons
> > correctly.
> I don't think bug 878805 will add any new css. So if you need some, please
> add it.
> 

This was just a note that an issue in bug 539837 can be completed post bug 878805, which I will note there..

(In reply to Richard Marti [:Paenglab] from comment #9)
> (In reply to :aceman from comment #8)
> > Actually bug 878805 proposes to make this change ("file here" -> folder
> > name) also in all the other pickers. So depends on which direction you want
> > this to be addressed.
> 
> I showed only the difference. The direction "file here" -> folder name would
> for me make sense. At the beginning of my TB usage I asked me sometimes,
> which folder is "choose this folder". This change could help not so
> experienced users.

I'm still confused by it!

Comment 11

4 years ago
Comment on attachment 757435 [details] [diff] [review]
patch

Review of attachment 757435 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, it looks like you can also get rid of the mail/ css for folderTree and folderTreeChildren - <http://mxr.mozilla.org/comm-central/search?string=foldersTree&filter=^[^\0]*$> since that's the last binding using them.
Attachment #757435 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 12

4 years ago
Created attachment 758784 [details] [diff] [review]
patch for checkin


remove unnecessary css per comment.
Attachment #757435 - Attachment is obsolete: true
Attachment #758784 - Flags: ui-review+
Attachment #758784 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d031447ec78b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0

Updated

4 years ago
Duplicate of this bug: 886912
Duplicate of this bug: 442802
Blocks: 507669
Blocks: 420506

Updated

2 months ago
Depends on: 1347012
You need to log in before you can comment on or make changes to this bug.