Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#110 closed defect (fixed)

layer node updates text when this may be unwanted

Reported by: bartvde Owned by: tschaub
Priority: minor Milestone: 0.6
Component: GeoExt.tree.LayerNode Version: 0.5
Keywords: Cc:
State: Needs Discussion

Description

This is a sequel to ticket:96

Imagine the following use case: somebody creates a GeoExt.tree.LayerNode and provides a value for text in the constructor. This means the node will not take the layer.name as text.

However, when somebody changes the visibility of the layer node, a changelayer event is fired, and onStoreUpdate is run in LayerNode.js, so the text is updated with the layer name, but this is not what the developer will want.

How do we deal with this? In onStoreUpdate we currently don't know that the event was originated by a visibility change, and not a layer name change.

Should we not listen for layer name changes if the user provided a text in the constructor?

Comments welcome.

Attachments (3)

ticket110.patch (871 bytes) - added by bartvde 8 years ago.
possible patch
geoext-110.patch (1.7 KB) - added by ahocevar 8 years ago.
use information from the record to determine if a custom text was set on the node
110.patch (2.0 KB) - added by tschaub 7 years ago.
allow layer name to be set multiple times

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by bartvde

  • Component changed from data to widgets.tree
  • Owner set to ahocevar

Changed 8 years ago by bartvde

possible patch

comment:2 Changed 8 years ago by tschaub

  • Milestone changed from 0.5 to 0.6

Changed 8 years ago by ahocevar

use information from the record to determine if a custom text was set on the node

comment:3 Changed 8 years ago by ahocevar

Bart: I uploaded a patch which is probably a bit smarter. So if we start with a custom text, the text will not be updated. But if we set node.text = layer.name at some point, the text will start synchronizing. What do you think?

comment:4 Changed 8 years ago by bartvde

  • State changed from None to Commit

Andreas, right this makes more sense and looks good. Thanks. Please commit.

comment:5 Changed 8 years ago by ahocevar

  • Resolution set to fixed
  • Status changed from new to closed

(In [1228]) Only change the node text to a new layer name if the node's text was equal to the layer name before the modification. r=bartvde (closes #110)

comment:6 follow-up: Changed 8 years ago by tschaub

  • Resolution fixed deleted
  • State changed from Commit to Needs Discussion
  • Status changed from closed to reopened

I think this needs revisiting.

The record.modified object contains the original data values. If you change the layer title once, it works to check if the node equals the original and change it if so. However, now that the node text has successfully been changed by setting the layer name, you can't repeat the process (the node.text will never equal the record.modified["title"]).

Ideally, layer nodes are generated by the container. To change the text, you change the layer title. Is there a situation where you really need the node text to be different than the layer title (layer.name or record.get("title"))?

comment:7 in reply to: ↑ 6 Changed 8 years ago by ahocevar

Replying to tschaub:

The record.modified object contains the original data values. If you change the layer title once, it works to check if the node equals the original and change it if so. However, now that the node text has successfully been changed by setting the layer name, you can't repeat the process (the node.text will never equal the record.modified["title"]).

record.modified contains the original data values 'since the last commit'. So if we call store.commitChanges(); after changing a title, everything will be fine. Not sure if this is the desired solution though.

comment:8 Changed 8 years ago by ahocevar

Instead of doing store.commitChanges();, record.commit(); is maybe better to avoid committing changes that have been made elsewhere.

comment:9 follow-up: Changed 8 years ago by bartvde

Hey Tim, if we are talking probability of use cases here anyway :-) , what is the chance that a layer name will be changed twice in an application?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by tschaub

Replying to bartvde:

Hey Tim, if we are talking probability of use cases here anyway :-) , what is the chance that a layer name will be changed twice in an application?

Do you really think it is good behavior that record.setName only works once? This is completely absurd to me. This came up in an app of ours where the app config had a title specified. The app called record.setName before the layer was added to the map. By the time the user even had a chance to set a new name, things were broken (their first attempt was the second call to record.setName).

I think the fixedName solution is acceptable (or something else similar). If a better one is not suggested, I say we implement that or revert the change that broke things.

comment:11 in reply to: ↑ 10 Changed 8 years ago by bartvde

Replying to tschaub:

Replying to bartvde:

Hey Tim, if we are talking probability of use cases here anyway :-) , what is the chance that a layer name will be changed twice in an application?

Do you really think it is good behavior that record.setName only works once? This is completely absurd to me. This came up in an app of ours where the app config had a title specified. The app called record.setName before the layer was added to the map. By the time the user even had a chance to set a new name, things were broken (their first attempt was the second call to record.setName).

I think the fixedName solution is acceptable (or something else similar). If a better one is not suggested, I say we implement that or revert the change that broke things.

Tim, thanks for explaining how this could break things. I hadn't thought of that particular use case. I am okay with the fixedName solution. Andreas?

comment:12 follow-up: Changed 8 years ago by ahocevar

I'm fine with either approach. But at some point, we should start thinking about how to use dirty/commit mechanisms from Ext. And as I showed above, committing a title change to the store will also to change titles multiple times.

comment:13 in reply to: ↑ 12 Changed 8 years ago by tschaub

Replying to ahocevar:

I'm fine with either approach. But at some point, we should start thinking about how to use dirty/commit mechanisms from Ext. And as I showed above, committing a title change to the store will also to change titles multiple times.

I fully agree that we should make use of commit. However, I think we are treating the node like a view of the record in this case. Calling record.set twice doesn't keep a grid row from updating. If we only want the node text to update on a commit, then the node text should update on a commit, not an update. Also, I think record.commit is meant to be called by store.commitChanges.

comment:14 Changed 7 years ago by tschaub

  • Owner changed from ahocevar to tschaub
  • Status changed from reopened to new

Changed 7 years ago by tschaub

allow layer name to be set multiple times

comment:15 Changed 7 years ago by tschaub

I think the latest patch reflects the decision we've arrived at here. It will be good to properly deal with dirty records when we bring in Ext 3.0. It also makes sense to allow people to set the layer title as many times as they want without committing anything and have the node updated.

I think we're split on how layer nodes should be considered. This patch reflects that split. If you see the node as a view of the data (layer), then it doesn't make sense to change things on the view. If you don't see the node as a view of the data, then you can provide custom text for the node and you don't care about changes to the data (hence, fixedText).

If we come to a common vision for this in the future, perhaps we can remove the fixedText property (or generalize it to a frozen view or something).

comment:16 Changed 7 years ago by tschaub

  • Resolution set to fixed
  • Status changed from new to closed

(In [1392]) Going with a compromise for layer nodes with regard to layer titles. If you want the layer node to display the layer title, don't construct a node a text property. If you don't want the node to be a view of the layer (and display any updated title), then construct the node with text. r=me (closes #110)

comment:17 Changed 7 years ago by tschaub

  • Component changed from GeoExt.tree to GeoExt.tree.LayerNode
Note: See TracTickets for help on using tickets.