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

Projects
None yet
2 participants
@Dayof
Contributor

Dayof commented Jul 17, 2017

@Dayof

This comment has been minimized.

Show comment
Hide comment
@Dayof

Dayof Jul 24, 2017

Contributor

This new API enables the creation of a tree through:

Contributor

Dayof commented Jul 24, 2017

This new API enables the creation of a tree through:

@Dayof

This comment has been minimized.

Show comment
Hide comment
@freakboy3742

This comment has been minimized.

Show comment
Hide comment
@freakboy3742

freakboy3742 Jul 25, 2017

Member

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?

Member

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

This comment has been minimized.

Show comment
Hide comment
@Dayof

Dayof Jul 25, 2017

Contributor

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?
Contributor

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?
@freakboy3742

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 pybee:master Sep 2, 2017

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