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

UITreeView (& test method conventions) #2338

Merged
merged 17 commits into from
Jun 5, 2016
Merged

UITreeView (& test method conventions) #2338

merged 17 commits into from
Jun 5, 2016

Conversation

rzats
Copy link
Member

@rzats rzats commented Jun 3, 2016

Contains

image

  • A Tree data structure that contains the widget's elements and a TreeModel helper class.
  • TreeViewTestScreen - fairly self-explanatory 😜
  • A codestyle fix renaming all the test methods to testMethodName as opposed to methodName - this is a mostly unrelated change as suggested in a Slack discussion.

This PR was previously located in my Terasology fork. In retrospect, this wasn't a great idea: local PRs have no @GooeyHub testing, no mergeability checks with the upstream repository and little to none community exposure: IMO, going for a base repository fork is a better idea for subsequent deliverables.

How to test

Controls

  • Mouse wheel: scroll up/down.
  • LMB: select/deselect a node.
  • RMB: expand/collapse a node.
  • Delete: remove a node (and all its' children).
  • Up/Down: change a node's position within the parent node.
  • Ctrl+C: copy a selected node.
  • Ctrl+V: paste the copied node as a child of the currently selected node.
  • Ctrl+A / Insert: add a new child with a placeholder value to the currently selected node.

@rzats rzats added Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience GSOC labels Jun 3, 2016
@GooeyHub
Copy link
Member

GooeyHub commented Jun 3, 2016

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

GooeyHub commented Jun 3, 2016

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

GooeyHub commented Jun 4, 2016

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

GooeyHub commented Jun 4, 2016

Hooray Jenkins reported success with all tests good!

@GooeyHub
Copy link
Member

GooeyHub commented Jun 4, 2016

Hooray Jenkins reported success with all tests good!

* @return The index of the specified {@code Tree}.
*/
public int getIndex(Tree<T> node) {
if (node == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Preconditions.checkArgument() here, if you like.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed the notification for this comment. Fixed all the checks accordingly.

@msteiger
Copy link
Member

msteiger commented Jun 5, 2016

I looked over it briefly and didn't test all corner cases, but afaict, it looks good.

Slightly offtopic: The behavior node editor currently uses org.abego.treelayout.TreeLayout to organize tree structured UI elements on the screen. Does it make sense to connect it with the code in this PR or can we consider those two as completely separate things?

@rzats
Copy link
Member Author

rzats commented Jun 5, 2016

@msteiger: thanks for the test!

I've actually checked the BTE package before starting work on this PR - it's specifically meant for tree visualizations as opposed to tree views, so not a great fit.
Tree could theoretically be modified to implement org.abego.treelayout.TreeForTreeLayout, although I don't see any benefits from doing so.

@msteiger
Copy link
Member

msteiger commented Jun 5, 2016

That's what I expected. Thanks for the clarification.

@GooeyHub
Copy link
Member

GooeyHub commented Jun 5, 2016

Hooray Jenkins reported success with all tests good!

@flo
Copy link
Contributor

flo commented Jun 5, 2016

@rzats got a IndexOutOfBoundException while testing it:
http://pastebin.com/hGHbfPvH

@flo
Copy link
Contributor

flo commented Jun 5, 2016

The crash happens when you minimize a node near the end of the list while a child node is still selected. When it isn't near the end of the list, another item gets selected (which is also strange).

@flo
Copy link
Contributor

flo commented Jun 5, 2016

Great work otherwise, the PR can be merged once the bug is fixed.

@flo flo merged commit 2edcf48 into MovingBlocks:develop Jun 5, 2016
@flo
Copy link
Contributor

flo commented Jun 5, 2016

Merged, thanks for the PR

@GooeyHub
Copy link
Member

GooeyHub commented Jun 5, 2016

Hooray Jenkins reported success with all tests good!

@rzats rzats mentioned this pull request Jun 5, 2016
10 tasks
@Cervator Cervator added this to the Alpha 2 milestone Jun 5, 2016
@rzats rzats deleted the feature-UITreeView branch June 5, 2016 21:38
@rzats rzats self-assigned this Jul 7, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants