Closed Bug 878604 Opened 11 years ago Closed 11 years ago

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

Categories

(Thunderbird :: Search, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 24.0

People

(Reporter: alta88, Assigned: alta88)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

      No description provided.
Attached patch patch (obsolete) — Splinter Review
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)
Attached patch patch (obsolete) — Splinter Review
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)
Nice.
But not sure what you mean with the Filter dialog. That one was already converted.
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).
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+
Attached patch patch (obsolete) — Splinter Review
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)
(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.
(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 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+
remove unnecessary css per comment.
Attachment #757435 - Attachment is obsolete: true
Attachment #758784 - Flags: ui-review+
Attachment #758784 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d031447ec78b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0
Depends on: 1347012
You need to log in before you can comment on or make changes to this bug.