670561 - show profile path in profile manager

Status: RESOLVED FIXED

(SeaMonkey :: Startup & Profiles, defect)

Description

[Karsten Düsterloh]

Show profile path in profile manager when hovering over a listbox entry like toolkit does: see <http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/profile/content/profileSelection.js#79>

Comment 1

[therube]

(Perhaps you'll be shot down for similar reasons, Bug 658186 - RFE: about:support Provide "Profile Directory" Path/Name in addition to Open Button ?)

But considering you're only looking for a mouseover.

I like the idea :-)

Comment 2

[Karsten Düsterloh]

This is pure SeaMonkey code, we don't have morons here. ;-)

Comment 3

[Stefan [:stefanh]]

Do we need to show it in prefs if we show it in profile manager?

Comment 4

[Stefan [:stefanh]]

(In reply to comment #3)
> Do we need to show it in prefs if we show it in profile manager?

Erm, forget what I said... I'm trying to honour "Think before you type", but here I failed...

Comment 5

[Edmund Wong (:ewong)]

Attachment: Show profile path in profile manager.

Comment 6

[Edmund Wong (:ewong)]

Attachment: Show profile path in profile manager.(v2)

Removed commented code.

Comment 7

[Ian Neal]

Comment on attachment 546394 [details] [diff] [review]
Show profile path in profile manager.(v2)

>+++ b/suite/common/profile/profileSelection.js
>   var selectedProfile = null;
>+  var profileName = '';
This should probably be called profile and no need to set to = ''
>+  var profileDir = '';
No need to set to = ''
>+  var profileInfo = new Object();
I don't think we need to complicate things by having this variable.

>   while (profileEnum.hasMoreElements()) {
>-    AddItem(profileEnum.getNext().QueryInterface(Components.interfaces.nsIToolkitProfile),
>-            selectedProfile);
>+    profileName = profileEnum.getNext().QueryInterface(Components.interfaces.nsIToolkitProfile);
From above, just call this variable profile.
>+    profileDir = gProfileService.getProfileByName(profileName.name).localDir;
>+    profileInfo.profile = profileName;
>+    profileInfo.profileDir = profileDir;
>+    AddItem(profileInfo, selectedProfile);
Just pass both profile and profileDir as arguments instead of creating the object.

> function AddItem(aProfile, aProfileToSelect)
So this would have aProfile, aProfileDir and aProfileToSelect.
> {
>   var tree = document.getElementById("profiles");
>   var treeitem = document.createElement("treeitem");
>   var treerow = document.createElement("treerow");
>   var treecell = document.createElement("treecell");
>-  treecell.setAttribute("label", aProfile.name);
>+  treecell.setAttribute("label", aProfile.profile.name);
This line would no longer have to be changed.
>+  treeitem.setAttribute("tooltip", aProfile.profileDir.persistentDescriptor);
This would have to be changed still but using aProfileDir instead.
>   treerow.appendChild(treecell);
>   treeitem.appendChild(treerow);
>   tree.lastChild.appendChild(treeitem);
>-  treeitem.profile = aProfile;
>-  if (aProfile == aProfileToSelect) {
>+  treeitem.profile = aProfile.profile;
>+  if (aProfile.profile == aProfileToSelect) {
These two lines would no longer have to be changed.

Comment 8

[Edmund Wong (:ewong)]

Attachment: Show profile path in profile manager.(v3)

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 546417 [details] [diff] [review]
Show profile path in profile manager.(v3)

Looks like a good approach to me, you'll want review from someone more familiar with this code though...

Comment 10

[[e-mail]]

Comment on attachment 546417 [details] [diff] [review]
Show profile path in profile manager.(v3)

>+    profileDir = gProfileService.getProfileByName(profile.name).localDir;
This should be the .rootDir, which is the main profile directory; the .localDir is only used for the various caches.

>+  treeitem.setAttribute("tooltip", aProfileDir.persistentDescriptor);
persistentDescriptor is an internal method used for localised file preferences. Its value should not be used in the UI! You probably want to use .path instead.

>+function HandlePopEvent(aEvent)
[Scope for bikeshedding]

>+  var toolTip = aEvent.target.triggerNode.children.item(row.value).tooltip;
tree.view.getItemAtIndex(row.value).tooltip

>+  treeTip.firstChild.value = toolTip;
Just set the .label directly on the tooltip itself. See below.

>+      <tooltip id="treetip"
>+               onpopupshowing="HandlePopEvent(event);">
>+        <label value=""/>
tooltips already contain a label by default; only people who need something more sophisticated need to add their own children.

Comment 11

[Karsten Düsterloh]

Comment on attachment 546417 [details] [diff] [review]
Show profile path in profile manager.(v3)

Nothing serious, just some nits:

>+  var profile;
>+  var profileDir;

I don't see the point of these two variables, because …

>-    AddItem(profileEnum.getNext().QueryInterface(Components.interfaces.nsIToolkitProfile),
>-            selectedProfile);
>+    profile = profileEnum.getNext().QueryInterface(Components.interfaces.nsIToolkitProfile);
>+    profileDir = gProfileService.getProfileByName(profile.name).localDir;
>+    AddItem(profile, profileDir, selectedProfile);

… calculating profileDir here just to pass it down to AddItem is nonsense. 
Just extract the dir inside AddItem.

>+function AddItem(aProfile, aProfileDir, aProfileToSelect)

No need to change the function signature, see above.

>+function HandlePopEvent(aEvent)
>+{
>+  var treeTip = document.getElementById("treetip");
>+  var tree = document.getElementById("profiles");
>+  var elt = {};
>+  var row = {};
>+  var col = {};
>+  
>+  tree.treeBoxObject.getCellAt(aEvent.clientX, aEvent.clientY, row, col, elt);

You can replace col and elt by using {} in the call directly, since you don't use them anyway.

>+  var toolTip = aEvent.target.triggerNode.children.item(row.value).tooltip;
>+  treeTip.firstChild.value = toolTip;

Permission granted to drop the toolTip variable without line break. ;-)

Comment 12

[Edmund Wong (:ewong)]

Attachment: Show profile path in profile manager. (v4)

Comment 13

[Karsten Düsterloh]

Comment on attachment 572202 [details] [diff] [review]
Show profile path in profile manager. (v4)

>+++ b/suite/common/profile/profileSelection.js
…
> function AddItem(aProfile, aProfileToSelect)
…
 function CreateProfile(aProfile)
> {
>   gProfileService.flush();
>-  AddItem(aProfile, aProfile);
>+  AddItem(aProfile, aProfile, aProfile);
> }

This change is not needed anymore, the third parameter is ignored anyway.

r=me with that.

Comment 14

[Edmund Wong (:ewong)]

Attachment: Show profile path in profile manager. (v5)

Comment 15

[Philip Chee]

Comment on attachment 574894 [details] [diff] [review]
Show profile path in profile manager. (v5)

Patch works. I see tooltips showing the path when hovering over individual profiles.

Comment 16

[Karsten Düsterloh]

Comment on attachment 574894 [details] [diff] [review]
Show profile path in profile manager. (v5)

(In reply to Karsten Düsterloh from comment #13)
> r=me with that.

This meant to mean „if you do (just) those changes, you don't need to ask me again, just set r+ yourself and go on“.
Of course, only if you feel comfortable with that. ;-)

Comment 17

[Edmund Wong (:ewong)]

Pushed to comm-central:

http://hg.mozilla.org/comm-central/rev/75669e449485

Comment 20

[[e-mail]]

Sadly this has a bug; if you hover over a blank area of the tree you get a blank tooltip, and additionally an assertion in debug builds.