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

Enhance tree search functionallity #3878

Merged
merged 26 commits into from
Mar 25, 2019

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Mar 11, 2019

This PR changes the search functionality in the trees tab view. It changes the search to a browser-like search where you can cycle through the found results via buttons or via pressing enter. The searched options are the name of the trees and the tree groups and the search should be case insensitive.

URL of deployed dev instance (used for testing):

Steps to test:

  • open a skeleton or hybrid tracing
  • create some trees and groups
  • use the search button or the shortcut shift + ctrl + f to open the search popover
  • opening the popover should automatically focus the search input of the popover
  • enter some keywords to search for
  • if there is no result, hitting enter and using the buttons should not select a new tree or group as active and the text should be colored red
  • when having multiple results, pressing enter or one of the buttons on the right side should cycle through the possible results without closing the search popover
  • the popover can be closed with pressing escape, clicking somewhere else (not on the popover) or using the close button on the right side of the popover
  • reopening the popover should keep the previously entered search sequence and index of "active search result" so that the user can continue to cycle through the results

Issues:


@MichaelBuessemeyer MichaelBuessemeyer changed the title [WIP] Enhance tree search functionallity Enhance tree search functionallity Mar 12, 2019
@@ -337,21 +346,37 @@ class TreesTabView extends React.PureComponent<Props, State> {
}
};

getAllSubtreeIdsOfGroup = (groupId: number): Array<number> => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just found out that this method is never used so I removed it

@MichaelBuessemeyer
Copy link
Contributor Author

@daniel-wer This PR is ready for your review 🙂

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Awesome, this already feels like a big improvement and the code looks good too!

Some observations, things to do:

  • Investigate performance in tracings with many trees (1000+).
  • Ideally the shortcut would be disabled if the tree tab is not visible. Do you have an idea how to accomplish that?
  • Should the search results be in the "correct" order (same as in the tree tab)? I'm thinking yes, after testing this, but it may be hard to accomplish.

const { data, searchKey, provideShortcut, children } = this.props;
const { searchQuery, isVisible } = this.state;
let { currentPosition } = this.state;
currentPosition = currentPosition == null ? -1 : currentPosition;
Copy link
Member

Choose a reason for hiding this comment

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

If trees are deleted while a searchQuery is entered, the currentPosition can become larger than numberOfAvailableOptions

too-many-trees

I think a modulo would be needed for the currentPosition here.

const { searchQuery, isVisible } = this.state;
let { currentPosition } = this.state;
currentPosition = currentPosition == null ? -1 : currentPosition;
this.availableOptions = this.getAvailableOptionsFrom(data, searchQuery, searchKey);
Copy link
Member

Choose a reason for hiding this comment

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

A react render method should be pure and should not have side effects. Easy to fix as the memoization allows us to simply call this.getAvailableOptionsFrom everywhere we need the availableOptions, so we don't need the instance variable :)

@daniel-wer
Copy link
Member

I fixed those last two comments (and some very minor stuff) by myself, hope that's alright :)

@daniel-wer
Copy link
Member

@MichaelBuessemeyer I just pushed a commit implementing the sorting of the groups and trees for the advanced search popover. I think the solution is not too complex and works nicely, please have a look and tell me what you think :)

@MichaelBuessemeyer
Copy link
Contributor Author

I fixed those last two comments (and some very minor stuff) by myself, hope that's alright :)

Thanks for doing that @daniel-wer 👍

I think a modulo would be needed for the currentPosition here.

I check this version and it works alright. But the behavior is kinda weird:
e.g.: You have 10 available trees and the currentPosition is also the 10th tree. Removing the 10th tree will cause the currentPosition to be the first tree. But I think it would make more sense if it would be the last currently available option (here the 9th tree). I achieved this by using Math.min: currentPosition = Math.min(currentPosition, numberOfAvailableOptions - 1);.

Please check this change for bugs :)

@MichaelBuessemeyer
Copy link
Contributor Author

@MichaelBuessemeyer I just pushed a commit implementing the sorting of the groups and trees for the advanced search popover. I think the solution is not too complex and works nicely, please have a look and tell me what you think :)

Looks and works awesome! 👍

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

I just did a last test, worked perfectly :)

Really good work on this PR! Ready to merge imo 👍

@MichaelBuessemeyer MichaelBuessemeyer merged commit 27a8e4f into master Mar 25, 2019
hotzenklotz added a commit that referenced this pull request Mar 25, 2019
…ture-highlight

* 'master' of github.com:scalableminds/webknossos:
  Hide unreported datasets (#3883)
  Update puppeteer and refresh screenshots (#3914)
  only show team names of own organization (#3928)
  Enable merger mode in skeleton and hybrid tracings (#3619)
  allow uploading nml for public dataset of different orga (#3929)
  Always make wheel listeners not passive to allow preventDefault (#3939)
  Enhance tree search functionallity (#3878)
  add webknossos-connect to setup (#3913)
  Update README.md (#3923)
  Add shortcut to maximize golden layout panes (#3927)
  Perform bucket picking in web workers and other performance optimizations (#3902)
  remove alt text for abstract brain loading image (#3930)
  updated documentation front page (#3917)
@normanrz normanrz deleted the enhance-tree-search-functionallity branch August 12, 2019 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance search functionality (group names, counter, jumping)
2 participants