Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring of the Tree widget [Core][Cocoa] [WIP] #201

Merged
merged 35 commits into from Sep 2, 2017

Conversation

@Dayof
Copy link
Contributor

@Dayof Dayof commented Jul 17, 2017

Example code to test : https://gist.github.com/Dayof/450510e713fa76abee588b51eb742da7

@Dayof
Copy link
Contributor Author

@Dayof Dayof commented Jul 24, 2017

This new API enables the creation of a tree through:

@Dayof
Copy link
Contributor Author

@Dayof Dayof commented Jul 24, 2017

@freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Jul 25, 2017

The API design here looks awesome. My only comment would be about the entry points on case 3 (an independent data source) - If I'm reading that correctly, the three required entry points for an independent data source are:

  • roots()
  • has_children_by_node()
  • children_by_node()

The other methods in the example code are required because of the way the example is constructed. Have I understood this correctly? If so:

  • We can probably simplify the naming - has_children and children is probably sufficient
  • Is there a reason you need both a has_children and children entry point? Isn't has_children just children() is not None?
  • Would it be possible to put these "functional" parts of the API behind an attribute to separate the namespace? The use case for (3) is when you have a data structure that you want to render on a tree. On such a data structure, it wouldn't be unusual to have a children attribute - which means you then have a name collision between the API you want to use natively, and the API that Toga requires as a data source. An option here is to require that a "Toga data source" doesn't access self.children() directly, but looks for a self._data.children(). In the simple case, you could make the _data attribute return self; but in the more complex case, you could completely separate the data source from the API Toga requires for that data source.

Does that make sense?

@Dayof
Copy link
Contributor Author

@Dayof Dayof commented Jul 25, 2017

Thanks for the reviewing @freakboy3742 !

  • I've made the modification about the entry points names e the necessity about the has_children_by_node() on the last commit.
  • The last part I dind't understand very well, if on the Toga side I set self._data = tree and then access self._data.children() to interact with the class constructed by the user, would have a problem?

Copy link
Member

@freakboy3742 freakboy3742 left a comment

I'll probably do some tweaking and cleanup to this, but what's here is good enough for trunk, so 👍

@freakboy3742 freakboy3742 merged commit f356e8d into beeware:master Sep 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants