Convert 'Customize Titlebar' dialog's listbox into tree, and make it's columns resizable. Fixes #25. #74

Closed
wants to merge 6 commits into from

2 participants

@xabolcs

Hi!

In this pull request I did the following:

  • convert the container from listbox to tree
    • put a description to the end to be a placeholder - w/o it, the vbox undersized itself and the lower border of the tree was hiding.
    • introduce a new customTreeView object in paneTitle
    • introduce the new setupTree TreeView initializer method in paneTitle
    • hack a bit to skip some onClick event originating from scrollbar
  • make tree's columns resizable with a default 1:1:2 ratio
  • extend tree default width height

The resolution is based on some copy-pasted MDN example. Feel free to review it hard!
For example I don't know why I left the custom treeview code in paneTitle instead making it
a local function in setupTree.

@xabolcs

I've updated the commits:

  • not manipulating tree width
  • moved treeView to a local variable
@whimboo whimboo and 1 other commented on an outdated diff Jun 3, 2012
extension/chrome/content/titlebar/customize.xul
@@ -64,20 +64,17 @@
<description style="padding-bottom: 5px">&nightly.variables.description;</description>
- <listbox id="varList" flex="1">
- <listhead>
- <listheader label="&nightly.variable.label;"/>
- <listheader label="&nightly.variabledesc.label;"/>
- <listheader label="&nightly.variablevalue.label;"/>
- </listhead>
- <listcols>
- <listcol style="width: 9em"/>
- <listcol flex="1"/>
- <listcol style="width: 15em"/>
- </listcols>
- </listbox>
+ <tree id="varList" flex="1" style="height: 17em" hidecolumnpicker="true">
@whimboo
whimboo added a note Jun 3, 2012

I would change the id of the tree to something more descriptive and which is loosely bound to the type of object, like 'variables'. Also I wouldn't set a height but would it auto-adjust by the window size. It should be stored inside of localstore.rdf so it keeps the latest width/height across sessions.

@xabolcs
xabolcs added a note Jun 5, 2012

whimboo wrote

I would change the id of the tree to something more descriptive and which is loosely bound to the type of object, like 'variables'.

For example: variableList, variablesList, variablesTree?

Also I wouldn't set a height but would it auto-adjust by the window size.

Without style="height: xxxxx" or rows="xxx" <tree> gets collapsed! Try it! :)

It should be stored inside of localstore.rdf so it keeps the latest width/height across sessions.

This is irrelevant here. Resizing of the <tree> would be managed by the <prefpane>'s flexibility! See changes for customize.xul in #79! This issue is about columns! :P

@whimboo
whimboo added a note Jun 12, 2012

For example: variableList, variablesList, variablesTree?

I would say just expand to variableList.

Without style="height: xxxxx" or rows="xxx" gets collapsed! Try it! :)

In that case please move the style definition out into the css file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo and 1 other commented on an outdated diff Jun 3, 2012
extension/chrome/content/titlebar/customize.js
@@ -114,7 +105,46 @@ toggled: function()
var checkbox = document.getElementById("enableTitleBar");
var text = document.getElementById("customTitle");
text.disabled=!checkbox.checked;
-}
+},
+
+setupTree: function(){
+ var treeView = {
@whimboo
whimboo added a note Jun 3, 2012

So this is like a template? Wouldn't it be better to have it as a global variable in that file with a specific name?

I know that this file doesn't really obey the coding style but please keep it as best as possible, e.g. indentation, body of object member methods.

@xabolcs
xabolcs added a note Jun 5, 2012

whimboo wrote

So this is like a template? Wouldn't it be better to have it as a global variable in that file with a specific name?

Template? Template for what?
Should I move treeView() out of paneTitle() and add it an Array[] parameter? And use like tree.view = new treeView(paneTitle.data); ? (see sort.js at MDN)

I know that this file doesn't really obey the coding style but please keep it as best as possible, e.g. indentation, body of object member methods.

Should I file a separete issue about that or a just a commit here would be enough?

@xabolcs
xabolcs added a note Jun 5, 2012

xabolcs wrote

Should I file a separete issue about that or a just a commit here would be enough?

OK! Noted the double indentation. Thanks for pointing that out! :)
No need for extra commits, issues!

@whimboo
whimboo added a note Jun 12, 2012

Should I move treeView() out of paneTitle() and add it an Array[] parameter?

There is no need to define a new prototype for it. Just move the var declaration out into the global scope.

@whimboo
whimboo added a note Jun 12, 2012

Should I file a separete issue about that or a just a commit here would be enough?

If you can please fix it right away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo and 1 other commented on an outdated diff Jun 3, 2012
extension/chrome/content/titlebar/customize.js
+setupTree: function(){
+ var treeView = {
+ get rowCount() { return paneTitle.data.length },
+ getCellText: function(row,column){
+ return paneTitle.data[row][column.id];
+ },
+ setTree: function(treebox){ this.treebox = treebox; },
+ isContainer: function(row){ return false; },
+ isSeparator: function(row){ return false; },
+ isSorted: function(){ return false; },
+ getLevel: function(row){ return 0; },
+ getImageSrc: function(row,col){ return null; },
+ getRowProperties: function(row,props){},
+ getCellProperties: function(row,col,props){},
+ getColumnProperties: function(colid,col,props){},
+ cycleHeader: function(col, elem){},
@whimboo
whimboo added a note Jun 3, 2012

Do we have to define all those methods even when we do not make use of those?

@xabolcs
xabolcs added a note Jun 5, 2012

whimboo wrote

Do we have to define all those methods even when we do not make use of those?

At least cycleHeader() is mandatory. Will check for the others.

@xabolcs
xabolcs added a note Jun 5, 2012

xabolcs wrote

Will check for the others.

All other method is mandatory too, except getLevel(), tree.xml throws NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED if it miss something from nsITreeView.

In short: I wouldn't like to cut out getLevel().

@whimboo
whimboo added a note Jun 12, 2012

Ok, makes sense in this case. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo commented on an outdated diff Jun 3, 2012
extension/chrome/content/titlebar/customize.js
+ getCellProperties: function(row,col,props){},
+ getColumnProperties: function(colid,col,props){},
+ cycleHeader: function(col, elem){},
+ };
+
+ var tree = document.getElementById("varList");
+ tree.view = treeView;
+ tree.addEventListener("click", function(event) {
+ var tbo = tree.treeBoxObject;
+
+ // get the row, col and child element at the point
+ var row = { }, col = { }, child = { };
+ tbo.getCellAt(event.clientX, event.clientY, row, col, child);
+
+ // a workaround to skip clicks from scrollbar
+ if (treeView.selection.currentIndex == row.value) {
@whimboo
whimboo added a note Jun 3, 2012

For comparisons please always make use of triple-operators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo and 1 other commented on an outdated diff Jun 3, 2012
extension/chrome/content/titlebar/customize.js
+ cycleHeader: function(col, elem){},
+ };
+
+ var tree = document.getElementById("varList");
+ tree.view = treeView;
+ tree.addEventListener("click", function(event) {
+ var tbo = tree.treeBoxObject;
+
+ // get the row, col and child element at the point
+ var row = { }, col = { }, child = { };
+ tbo.getCellAt(event.clientX, event.clientY, row, col, child);
+
+ // a workaround to skip clicks from scrollbar
+ if (treeView.selection.currentIndex == row.value) {
+ var titlebox = document.getElementById("customTitle");
+ var template = titlebox.value + " " + paneTitle.data[row.value]["variable"];
@whimboo
whimboo added a note Jun 3, 2012

Something we should improve here is to let users add variables not only at the end of the string but at the current cursor position. I'm happy with a follow-up for this RFE.

@xabolcs
xabolcs added a note Jun 5, 2012

whimboo

... I'm happy with a follow-up for this RFE.

Mee too! :) Filed #80.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo

In general it looks fine and I agree that a tree will give us more changes in the future. I haven't tested this code yet but will do it once my above comments have been addressed.

@xabolcs

whimboo wrote

... I haven't tested this code yet but will do it once my above comments have been addressed.

Commits updated with addressed comments:

  • === comparison
  • tree's id=varList renamed to variablesTree
  • tree sizing unchanged, see comments!
  • treeView() moved to a (class-like?) global object: arrayBasedTreeView()
  • paneTitle.data[] renamed to paneTitle.variables[]
  • corrected indentation in setupTree()
@whimboo whimboo commented on an outdated diff Jun 12, 2012
extension/chrome/content/titlebar/customize.js
@@ -35,9 +35,76 @@
*
* ***** END LICENSE BLOCK ***** */
+function arrayBasedTreeView (treeViewData) {
+ this.data = treeViewData;
+}
+
+arrayBasedTreeView.prototype = {
+
+data: [],
+
+get rowCount()
+{
+ return this.data.length
+},
@whimboo
whimboo added a note Jun 12, 2012

Please obey the 2 char indentation for prototype methods and properties. Also the opening brackets should always be in the same line as the function name. And there is a missing semicolon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo commented on an outdated diff Jun 12, 2012
extension/chrome/content/titlebar/customize.js
@@ -114,7 +171,28 @@ toggled: function()
var checkbox = document.getElementById("enableTitleBar");
var text = document.getElementById("customTitle");
text.disabled=!checkbox.checked;
-}
+},
+
+setupTree: function(){
+ var tree = document.getElementById("variablesTree");
+ tree.view = new arrayBasedTreeView(paneTitle.variables);
+ tree.addEventListener("click", function(event) {
+ var tbo = tree.treeBoxObject;
@whimboo
whimboo added a note Jun 12, 2012

Mind moving this callback method into the global scope? You will have access to the tree via event.originalTarget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo commented on an outdated diff Jun 12, 2012
extension/chrome/content/titlebar/customize.js
+ var tbo = tree.treeBoxObject;
+
+ // get the row, col and child element at the point
+ var row = { }, col = { }, child = { };
+ tbo.getCellAt(event.clientX, event.clientY, row, col, child);
+
+ // a workaround to skip clicks from scrollbar
+ if (tree.view.selection.currentIndex === row.value) {
+ var titlebox = document.getElementById("customTitle");
+ var template = titlebox.value + " " + paneTitle.variables[row.value]["variable"];
+ titlebox.value = template;
+ // manually set pref, pref change isn't triggered if we just set the value
+ paneTitle.nightly.preferences.setCharPref("templates.title", template);
+ }
+ }, true);
+},
}
window.addEventListener("load",paneTitle.init,false);
@whimboo
whimboo added a note Jun 12, 2012

Beside note: I think we should remove the listener in paneTitle.init(). It's not necessary anymore once the window has been initialized. It will save a bit of memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo and 1 other commented on an outdated diff Jun 12, 2012
extension/chrome/content/titlebar/customize.xul
@@ -64,20 +64,17 @@
<description style="padding-bottom: 5px">&nightly.variables.description;</description>
- <listbox id="varList" flex="1">
- <listhead>
- <listheader label="&nightly.variable.label;"/>
- <listheader label="&nightly.variabledesc.label;"/>
- <listheader label="&nightly.variablevalue.label;"/>
- </listhead>
- <listcols>
- <listcol style="width: 9em"/>
- <listcol flex="1"/>
- <listcol style="width: 15em"/>
- </listcols>
- </listbox>
+ <tree id="variablesTree" flex="1" style="height: 17em" hidecolumnpicker="true">
@whimboo
whimboo added a note Jun 12, 2012

Personally I would say we should remove hidecolumnpicker="true" and let the user decide which columns he is interested in.

@xabolcs
xabolcs added a note Jun 13, 2012

As talked on IRC, will leave this attribute as is.
If there is a way to control the value of hidecolumnpicker programatically then a follow-up issue could be filed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo

Ok, so I will wait for an update for the other items.

@xabolcs xabolcs Issue #25 - addressing comments, part 1:
- coding style
- semicolon
- onclick listener
- onload listener
- tree height

--HG--
rename : extension/chrome/skin/options/options.css => extension/chrome/skin/titlebar/titlebar.css
7febb0d
@whimboo whimboo commented on an outdated diff Jul 2, 2012
extension/chrome/content/titlebar/customize.js
@@ -35,12 +35,68 @@
*
* ***** END LICENSE BLOCK ***** */
+function arrayBasedTreeView (treeViewData) {
+ this.data = treeViewData;
+}
+
+arrayBasedTreeView.prototype = {
+ data: [],
+
+ get rowCount () {
@whimboo
whimboo added a note Jul 2, 2012

Nit: this is a named method, so no space is necessary before the opening brackets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo commented on an outdated diff Jul 2, 2012
extension/chrome/content/titlebar/customize.js
{
+ aEvent.originalTarget.defaultView.window.removeEventListener("load", paneTitle.init, false);
@whimboo
whimboo added a note Jul 2, 2012

Why do we need defaultView.window? defaultView is already the window we want to remove the listener from, isn't it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo commented on the diff Jul 2, 2012
extension/chrome/skin/titlebar/titlebar.css
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+#variableTree {
+ height: 17em;
+}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
xabolcs added some commits Jul 9, 2012
@xabolcs xabolcs Merge master to branch-issue-25-resize-column ba06511
@xabolcs xabolcs Issue #25 - addressed comments, part 2:
- move <prefwindow> styling to titlebar.css
- avoid defaultView.window
- no space between named function and "("
66c0392
@xabolcs

@whimboo
Added commits:

  • a merge commit to address the CSS styling
  • and another for the nits
@whimboo whimboo commented on an outdated diff Aug 26, 2012
extension/chrome/content/titlebar/customize.js
}
+
+function treeOnClickListener (aEvent) {
@whimboo
whimboo added a note Aug 26, 2012

nit: no need for a blank after the function name. It's no a noname one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo commented on an outdated diff Aug 26, 2012
extension/chrome/content/titlebar/customize.js
}
+
+function treeOnClickListener (aEvent) {
+ if (aEvent.originalTarget.tagName == "treechildren") {
@whimboo
whimboo added a note Aug 26, 2012

nit: for the new code please immediately use the triple operators. Others we can update later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@whimboo whimboo and 1 other commented on an outdated diff Aug 26, 2012
extension/chrome/skin/titlebar/titlebar.css
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+#variableTree {
+ height: 17em;
+}
+#NightlyTesterOptions {
+ width: 42.75em;
@whimboo
whimboo added a note Aug 26, 2012

nit: please add an empty line before this CSS declaration.

Something I forgot and don't know if we talked about already, why don't we need the height? Is it set automatically based on the number of maximum visible rows of the tree?

@xabolcs
xabolcs added a note Aug 26, 2012

Something I forgot and don't know if we talked about already, why don't we need the height?

Yes, I left it out. See below!

Is it set automatically based on the number of maximum visible rows of the tree?

Yes, it is set automatically.

@whimboo
whimboo added a note Aug 26, 2012

So with all the template entries we have right now, which height of the window do we get? I just want to be sure that we do not extend the screen height of the users machine. At least not for a resolution of 1024x768.

@xabolcs
xabolcs added a note Aug 26, 2012

xabolcs wrote

Is it set automatically based on the number of maximum visible rows of the tree?

Yes, it is set automatically.

Hmm. A height is set by #variableTree, so no height styling is needed in it's container #NightlyTesterOptions. It would be automatic.

whimboo wrote:

... which height of the window do we get? ...

42.75em is 684px (at 16px default font size). See EMCalc!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@xabolcs xabolcs Issue #25 - addressed comments, part 3:
- triple operator
- no space between named function and "(" again!
- empty line in CSS
5500fc8
@xabolcs

@whimboo
Added commit:

  • ===
  • no space between ...
  • and the CSS
@xabolcs xabolcs added a commit that referenced this pull request Oct 17, 2012
@xabolcs xabolcs Convert 'Customize Titlebar' dialog's listbox into tree, and make it'…
…s columns resizable (#74)
619675b
@whimboo whimboo closed this Oct 17, 2012
@xabolcs

Landed as 619675b - 619675b

@whimboo

There is no need for this additional comment. See the one before I closed the pull. It already has the link.

@xabolcs

Ohh. Really.
Sorry for spamming. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment