Closed Bug 1088222 Opened 5 years ago Closed 5 years ago

Use improved versions of getCellAt() and getCoordsForCellItem() introduced by Bug 979835

Categories

(SeaMonkey :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.34

People

(Reporter: philip.chee, Assigned: shubham, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js][mentor=RattyAway])

Attachments

(1 file, 2 obsolete files)

See: http://mxr.mozilla.org/comm-central/source/mozilla/dom/webidl/TreeBoxObject.webidl?rev=21ea52a1c31d&mark=154-175#154

>   [Throws]
>   TreeCellInfo getCellAt(long x, long y);

>   /**
>    * DEPRECATED: please use above version
>    */
>   [Throws]
>   void getCellAt(long x, long y, object row, object column, object childElt);

>   /**
>    * Find the coordinates of an element within a specific cell.
>    */
>   [Throws]
>   DOMRect? getCoordsForCellItem(long row, TreeColumn col, DOMString element);

>   /**
>    * DEPRECATED: Please use above version
>    */
>   [Throws]
>   void getCoordsForCellItem(long row, TreeColumn col, DOMString element,
>                             object x, object y, object width, object height);

For example:

>  -      var xObj = {}, yObj = {}, widthObj = {}, heightObj = {};
>  -      treeBoxObject.getCoordsForCellItem(1, tree.columns[0], "cell",
>  -                                         xObj, yObj, widthObj, heightObj);
>  +      var rect = treeBoxObject.getCoordsForCellItem(1, tree.columns[0], "cell");
>   
>         var treeAcc = getAccessible(tree, [nsIAccessibleTable]);
>         var cellAcc = treeAcc.getCellAt(1, 0);
>         var rowAcc = cellAcc.parent;
>   
>  -      var cssX = xObj.value + treeBodyBoxObj.x;
>  -      var cssY = yObj.value + treeBodyBoxObj.y;
>  +      var cssX = rect.x + treeBodyBoxObj.x;
>  +      var cssY = rect.y + treeBodyBoxObj.y;

And:

>         var tree = aDocument.tooltipNode.parentNode;
>  -      var row = {}, column = {};
>         var tbo = tree.treeBoxObject;
>  -      tbo.getCellAt(aEvent.clientX, aEvent.clientY, row, column, {});
>  -      if (row.value == -1)
>  +      var cell = tbo.getCellAt(aEvent.clientX, aEvent.clientY);
>  +      if (cell.row == -1)
>           return false;
>  -      node = tree.view.nodeForTreeIndex(row.value);
>  -      cropped = tbo.isCellCropped(row.value, column.value);
>  +      node = tree.view.nodeForTreeIndex(cell.row);
>  +      cropped = tbo.isCellCropped(cell.row, cell.col);
Hi,
I want to work on this bug. How should I proceed?
(In reply to Shubham Jindal from comment #1)
> Hi,
> I want to work on this bug. How should I proceed?

First make sure you can build SeaMonkey. See https://developer.mozilla.org/en-US/docs/Simple_SeaMonkey_build for some guidance.

Much of our communications is done over IRC. You should join irc://moznet/introduction which is where there are people who will help you build SeaMonkey successfully. Once that's achieved you should join us in irc://moznet/SeaMonkey and/or our developer newsgroup news://news.mozilla.org/mozilla.dev.apps.seamonkey

Further guidance will be provided therein.
Flags: needinfo?(shubhamjindal18)
I am trying to download comm-central using hg. But it is taking a lot of time. How much time is expected to get the download complete?
Flags: needinfo?(shubhamjindal18)
(In reply to Shubham Jindal from comment #3)
> I am trying to download comm-central using hg. But it is taking a lot of
> time. How much time is expected to get the download complete?

[We can take this to email or IRC.]

Probably faster if you download the mercurial bundles:

<https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code/Mercurial/Bundles>

You'll need both the comm-central and mozilla-central bundles. mozilla-central probably weighs in at 2gig these days.
We can discuss this over email..:) I have already sent you an email.
Hi Philip,
I have replaced the functions accordingly as mentioned by you in the first comment. I am facing a problem in the following code.

>  var row = { }, col = { }, obj = { };
>  tbo.getCellAt(aEvent.clientX, aEvent.clientY, row, col, obj);
>  if (row.value == -1 || obj.value == "twisty")
>    return;
>  

What should I write in place of obj.value now assuming the following code.

>  var cell = tbo.getCellAt(aEvent.clientX, aEvent.clientY);
>  if (cell.row == -1 || obj.value == "twisty")
>    return;
>
Attached patch bug1088222.diff (obsolete) — Splinter Review
Attachment #8533637 - Flags: review?(philip.chee)
Attached patch bug1088222.diff (obsolete) — Splinter Review
Attachment #8533637 - Attachment is obsolete: true
Attachment #8533637 - Flags: review?(philip.chee)
Attachment #8533736 - Flags: review?(philip.chee)
Assignee: nobody → shubhamjindal18
Status: NEW → ASSIGNED
Comment on attachment 8533736 [details] [diff] [review]
bug1088222.diff

>  function ClickOnOtherPanels(event)
....
>      var row = {}, col = {}, obj = {};
I think you can delete the above line ^^^

>    onTreeClick: function onTreeClick(event) {
....
> -    let row = {}, col = {};
> -    this.treeBox.getCellAt(event.clientX, event.clientY, row, col, {});
> -    if (col.value && col.value.id == "enabled")
> -      this.toggle(row.value);
> +    var cell = this.treeBox.getCellAt(event.clientX, event.clientY);
Stick to the prevailing style in this file and use "let" here instead of "var"

> -          var row = {};
> -          var col = {};
> -          var childElt = {};
> -          folderTree.treeBoxObject.getCellAt(event.clientX, event.clientY, row, col, childElt);
> -          row = row.value;
> -          col = col.value;
> +          var cell = folderTree.treeBoxObject.getCellAt(event.clientX, event.clientY);

> +          row = cell.row;
> +          col = cell.col;
If you don't define a variable before using it, it will get hoisted into the global scope. This probably isn't what you want to do. In addition, in strict mode this is an error.
You might do this:
          var row = cell.row;
          var col = cell.col;
But since Spidermonkey understands ES6 destructuring assignment syntax you should be able to do:
          var { row, col } = folderTree.treeBoxObject.getCellAt(event.clientX, event.clientY);

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment

> -    var row = {}, col = {}, childElt = {};
>      var filterTree = document.getElementById("filterTree");
> -    filterTree.treeBoxObject.getCellAt(event.clientX, event.clientY, row, col, childElt);
> -    if (row.value == -1 || row.value > filterTree.view.rowCount-1 || event.originalTarget.localName != "treechildren") {
> +    var cell = filterTree.treeBoxObject.getCellAt(event.clientX, event.clientY);
> +    if (cell.row == -1 || cell.row > filterTree.view.rowCount-1 || event.originalTarget.localName != "treechildren") {
Since you're touching this line, please put a space on each side of the "-" operator e.g. filterTree.view.rowCount - 1

> -    if (!col.value.cycler)
> +    if (!cell.col)
Somehow I don't think this is correct.

> -  var row = {}, col = {}, childElt = {};
> -  gSearchTreeBoxObject.getCellAt(event.clientX, event.clientY, row, col, childElt);
> -  if (row.value == -1 || row.value > gSearchView.rowCount-1)
> +  var cell = gSearchTreeBoxObject.getCellAt(event.clientX, event.clientY);
> +  if (cell.row == -1 || cell.row > gSearchView.rowCount-1)
Since you're touching this line, please put a space on each side of the "-" operator e.g. gSearchView.rowCount - 1
Attachment #8533736 - Flags: review?(philip.chee) → review-
Attached patch bug1088222.diffSplinter Review
Attachment #8533736 - Attachment is obsolete: true
Attachment #8536135 - Flags: review?(philip.chee)
Comment on attachment 8536135 [details] [diff] [review]
bug1088222.diff

Looks real good. r=me
Attachment #8536135 - Flags: review?(philip.chee) → review+
Don't we have an option of "checkin-needed" here?
(In reply to Shubham Jindal from comment #12)
> Don't we have an option of "checkin-needed" here?
Yes indeed we do.
Keywords: checkin-needed
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/ead55271f364
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.34
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.