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

Status: Add ability to filter files #339

Merged
merged 3 commits into from Aug 18, 2014

Conversation

Projects
None yet
2 participants
@mmargoliono
Contributor

mmargoliono commented Aug 16, 2014

In a big project with a lot of change spread across directory, it is
hard to search a specific for a file change.

This change will add ability to filter the files status similar to how
git status filtering.

Reported-by: hrj
Signed-off-by: Minarto Margoliono lie.r.min.g@gmail.com
Issue Number: #337

Status: Add ability to filter files
In a big project with a lot of change spread across directory, it is
hard to search a specific for a file change.

This change will add ability to filter the files status similar to how
git status filtering.

Reported-by: hrj
Signed-off-by: Minarto Margoliono <lie.r.min.g@gmail.com>
@mmargoliono

This comment has been minimized.

Show comment
Hide comment
@mmargoliono

mmargoliono Aug 16, 2014

Contributor

Do you think it is better to move the filtering widget to a dockable widget. This way we can show/hide it from the UI.

Contributor

mmargoliono commented Aug 16, 2014

Do you think it is better to move the filtering widget to a dockable widget. This way we can show/hide it from the UI.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Aug 16, 2014

Member

I like where you're going with this. I have a few suggestions.

We only allow filtering a single path. In the DAG tool the input field does auto-complete on both refs and paths. I think there should be a widget that handles auto-complete for paths only (or there should be a way to reuse that code). Take a look at cola/widgets/completion.py -- there's a GitLogCompletionLineEdit that completes for both refs and paths. I think the paths part can be refactored so that we can provide an additional GitPathsCompletionLineEdit for completing just paths and then use it in the filter widget.

When getting the value from the text field, we probably want to from cola.compat import unicode and use it when getting the text, e.g. unicode(self.path_filter.text()) so that we get back unicode strings rather than QStrings.

When passing around the path, change the signatures to be paths=None so that we can support filtering on multiple paths. The final function that applies it will need to do a if paths is not None check before using it and default to [] if the path is None. BTW, it's a python gotcha that we shouldn't use empty lists in function signatures, e.g. paths=[], because it creates a global variable that can cause subtle bugs, so we use None instead of [] to let functions know that the callee provided an empty list.

In order to get multiple paths from a single text field, call utils.shell_split() on the unicode string. That function supports shell syntax, which means we can support files with spaces in their name, etc. using the familiar unix shell syntax. Combined with the filename completion, that should make the filter able to support multiple paths and easier to use.

What do you think?

BTW, when I tried this on Mac OS X I kept getting segfaults.. my guess is that maybe it's the QString vs. unicode string thing, so let's try that first.

Once we've made these changes, I can help tweaking the GUI a little bit to make the filter widget less invasive. The suggestions below can come after we've adjusted the above notes.

Let me know if you'd like help with these:

  • The status widget can have additional buttons added to its header section (look for add_corner_widget(). I would suggest having a toggle button there that hides/shows the filter widget. We can find a good filter icon, maybe from the tango icon set, so that we can avoid needing to use text there. I can go find the original icon set where I got some of the other icons and bring over a filter icon if that would be helpful.
  • Having a good tooltip on the filter toggle icon will help make it more discoverable.
  • It's probably worth tweaking the layout to have a much smaller (or empty) margin so that the filter sits in better and is more snug alongside its sibling widgets.
  • I suggest using the same filter icon as in the top section to replace the Filter button text, or even better is not having a button at all. I like the idea of not having a button -- the action can be triggered when pressing enter or losing focus. Run bin/git-dag and then start entering the name of a file after the -- double-dash. The view is updated once you hit enter. I think it would be good to make the filter widget interaction consistent with how that widget works.

Thanks for taking this on! This is a pretty slick feature.

Member

davvid commented Aug 16, 2014

I like where you're going with this. I have a few suggestions.

We only allow filtering a single path. In the DAG tool the input field does auto-complete on both refs and paths. I think there should be a widget that handles auto-complete for paths only (or there should be a way to reuse that code). Take a look at cola/widgets/completion.py -- there's a GitLogCompletionLineEdit that completes for both refs and paths. I think the paths part can be refactored so that we can provide an additional GitPathsCompletionLineEdit for completing just paths and then use it in the filter widget.

When getting the value from the text field, we probably want to from cola.compat import unicode and use it when getting the text, e.g. unicode(self.path_filter.text()) so that we get back unicode strings rather than QStrings.

When passing around the path, change the signatures to be paths=None so that we can support filtering on multiple paths. The final function that applies it will need to do a if paths is not None check before using it and default to [] if the path is None. BTW, it's a python gotcha that we shouldn't use empty lists in function signatures, e.g. paths=[], because it creates a global variable that can cause subtle bugs, so we use None instead of [] to let functions know that the callee provided an empty list.

In order to get multiple paths from a single text field, call utils.shell_split() on the unicode string. That function supports shell syntax, which means we can support files with spaces in their name, etc. using the familiar unix shell syntax. Combined with the filename completion, that should make the filter able to support multiple paths and easier to use.

What do you think?

BTW, when I tried this on Mac OS X I kept getting segfaults.. my guess is that maybe it's the QString vs. unicode string thing, so let's try that first.

Once we've made these changes, I can help tweaking the GUI a little bit to make the filter widget less invasive. The suggestions below can come after we've adjusted the above notes.

Let me know if you'd like help with these:

  • The status widget can have additional buttons added to its header section (look for add_corner_widget(). I would suggest having a toggle button there that hides/shows the filter widget. We can find a good filter icon, maybe from the tango icon set, so that we can avoid needing to use text there. I can go find the original icon set where I got some of the other icons and bring over a filter icon if that would be helpful.
  • Having a good tooltip on the filter toggle icon will help make it more discoverable.
  • It's probably worth tweaking the layout to have a much smaller (or empty) margin so that the filter sits in better and is more snug alongside its sibling widgets.
  • I suggest using the same filter icon as in the top section to replace the Filter button text, or even better is not having a button at all. I like the idea of not having a button -- the action can be triggered when pressing enter or losing focus. Run bin/git-dag and then start entering the name of a file after the -- double-dash. The view is updated once you hit enter. I think it would be good to make the filter widget interaction consistent with how that widget works.

Thanks for taking this on! This is a pretty slick feature.

mmargoliono added some commits Aug 17, 2014

Status: Filter multiple path criteria inputs
The files filtering in status widget was only accepting single path
criteria. This is a bit inconvenience when you want to filter on
multiple path filter criteria.

This change will add ability to filter the files based on multiple
criteria separated by a space.

Reported-by: hrj
Signed-off-by: Minarto Margoliono <lie.r.min.g@gmail.com>
Status: Add autocompletion for the filter
To use the filter in status widget, a user need to know the exact path
they want to filter. This is a bit user unfriendly as the user might not
always remember the exact path they want to filter on.

This change will add autocomplete pop-up for filter in status widget. It
will autocomplete to both folders path and files path.

Reported-by: hrj
Signed-off-by: Minarto Margoliono <lie.r.min.g@gmail.com>
@mmargoliono

This comment has been minimized.

Show comment
Hide comment
@mmargoliono

mmargoliono Aug 17, 2014

Contributor

I implemented the multiple path criteria. I think that is a good idea. Let me know whether you still get the segfaults.

In regards to autocompletion, the current code in DAG tools listed all files and folder in repositories. This has a plus and minus on itself. Let's say I made a change in cola/widgets/completion.py. Do i want cola/models as one of the completion? Displaying it will be useful in the case i want to filter on that path for the purpose of confirming that there is no change for the models. On the other hand, it is a kind of noise as I will get lot of suggestion (most of them won't be really modified)

The reason I added the button was I didn't want to filter on every keydown as that might be expensive. Since now it is using autocompletion, the filtering can be done on completion event like you said. My only concern is, using the current approach I can filter using wildcard (i.e *models* or *.py). Is it still possible to do that if we use similar implementation to DAG implementation.

on an unrelated-topic, are you interested on a feature to show file history on DAG? I was thinking of having context menu in file difference widget, on selection, it will filter the dag to list all change involving those files. I will probably create a separate wishlist issue later.

Contributor

mmargoliono commented Aug 17, 2014

I implemented the multiple path criteria. I think that is a good idea. Let me know whether you still get the segfaults.

In regards to autocompletion, the current code in DAG tools listed all files and folder in repositories. This has a plus and minus on itself. Let's say I made a change in cola/widgets/completion.py. Do i want cola/models as one of the completion? Displaying it will be useful in the case i want to filter on that path for the purpose of confirming that there is no change for the models. On the other hand, it is a kind of noise as I will get lot of suggestion (most of them won't be really modified)

The reason I added the button was I didn't want to filter on every keydown as that might be expensive. Since now it is using autocompletion, the filtering can be done on completion event like you said. My only concern is, using the current approach I can filter using wildcard (i.e *models* or *.py). Is it still possible to do that if we use similar implementation to DAG implementation.

on an unrelated-topic, are you interested on a feature to show file history on DAG? I was thinking of having context menu in file difference widget, on selection, it will filter the dag to list all change involving those files. I will probably create a separate wishlist issue later.

@davvid davvid merged commit 810c6b6 into git-cola:master Aug 18, 2014

davvid added a commit that referenced this pull request Aug 18, 2014

Merge branch 'status-filtering'
Teach the status widget to filter paths.

* status-filtering:
  status: add a filter toggle button
  status: remove the "Filter" button
  fixup! completion: use hinted line edits as the base class
  status: add hint text to the filter widget
  completion: use hinted line edits as the base class
  widgets: use consistent margins across all widgets
  status: update filter when return is pressed
  completion: better double-dash handling for DAG-like inputs
  completion: use value() where appropriate
  utils: simplify add_parents() by operating on a copy
  status: add a specialized completion model for status filtering
  status: make sure we always pass None as the status filter
  Status: Add autocompletion for the filter
  Status: Filter multiple path criteria inputs
  Status: Add ability to filter files

Closes #337
Closes #339

Suggested-by: @hrf
Thanks-to: Minarto Margoliono <lie.r.min.g@gmail.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Aug 18, 2014

Member

Thanks! Check out the little tweaks I made to it, it's pretty slick.

@mmargoliono yes, file history on the DAG would be useful. Right now you can get it by typing e.g. "Makefile" into the input text and it'll filter the history to those paths, so it might already be supported. Let me know what you have in mind.

Member

davvid commented Aug 18, 2014

Thanks! Check out the little tweaks I made to it, it's pretty slick.

@mmargoliono yes, file history on the DAG would be useful. Right now you can get it by typing e.g. "Makefile" into the input text and it'll filter the history to those paths, so it might already be supported. Let me know what you have in mind.

@mmargoliono mmargoliono deleted the mmargoliono:status-filtering branch Aug 18, 2014

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