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

Code view: support glob filters #532

Merged
merged 37 commits into from Mar 6, 2019

Conversation

apoorva17
Copy link
Contributor

Fixes #504

With the addition of #170, users can filter the code view by filetype
by selecting tick boxes.

Let's support a more powerful search box where users can filter files
by glob e.g. **/test/*.java.

@apoorva17 apoorva17 force-pushed the 504-support-glob-filters branch 2 times, most recently from 4f780f7 to 99ee679 Compare February 1, 2019 15:36
With the addition of reposense#170, users can filter the code view by filetype
by selecting tick boxes.

Let's support a more powerful search box where users can filter files
by glob e.g. **/test/*.java.
@apoorva17
Copy link
Contributor Author

Since I could not find any frontend Javascript library for glob pattern matching, i have used a node-module called minimatch. Therefore, I have used browserify and vueify to use minimatch in the frontend.

Since the build javascript file produced is automated, it does not conform to ESLint style checks, therefore I have excluded it from the travis.yml file.

@apoorva17
Copy link
Contributor Author

@reposense/devs ready for review

@apoorva17
Copy link
Contributor Author

@ongspxm kindly review

Copy link
Contributor

@yamidark yamidark left a comment

Choose a reason for hiding this comment

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

This feature doesn't work together with the current file type filter checkboxes:
Uncheck all file types
Set glob filter to any of the filetypes (e.g. **.java)
All .java files will appear, when the checkbox for .java is still unchecked.

Copy link
Member

@eugenepeh eugenepeh left a comment

Choose a reason for hiding this comment

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

@apoorva17 I intend to have the file types selection to tap on the search bar functionality when its complete.
E.g. clicking on the file formats check box will update the search bar, letting the search bar to perform the filtration, so that we can remove the redundant code that does the same thing. Can you look into it?

@apoorva17
Copy link
Contributor Author

This feature doesn't work together with the current file type filter checkboxes:
Uncheck all file types
Set glob filter to any of the filetypes (e.g. **.java)
All .java files will appear, when the checkbox for .java is still unchecked.

@yamidark this is fixed now

@apoorva17
Copy link
Contributor Author

@apoorva17 I intend to have the file types selection to tap on the search bar functionality when its complete.
E.g. clicking on the file formats check box will update the search bar, letting the search bar to perform the filtration, so that we can remove the redundant code that does the same thing. Can you look into it?

This can be looked at in a separate PR. @damithc your opinion?

@damithc
Copy link
Collaborator

damithc commented Feb 8, 2019

This can be looked at in a separate PR. @damithc your opinion?

I'm OK to do it as a separate PR.

@eugenepeh Actually, I'm not entirely sure about that feature. Auto-updating the search bar as the user ticks checkboxes is nice, but complications will arise when the user types in the search bar and tick checkboxes in tandem. If we go in that direction, we may have to disable checkboxes the moment the user starts typing in the search bar.
Alternatively, instead of auto-updating the search bar, we can simply consider the combination of the two implicitly. i.e., if the documentation group is ticked and the user types *foo*.adoc in the search bar, we filter out files that meet both criteria.

@eugenepeh
Copy link
Member

eugenepeh commented Feb 8, 2019

@eugenepeh Actually, I'm not entirely sure about that feature. Auto-updating the search bar as the user ticks checkboxes is nice, but complications will arise when the user types in the search bar and tick checkboxes in tandem. If we go in that direction, we may have to disable checkboxes the moment the user starts typing in the search bar.

The reason I propose this is because it is not going to be easy to get the checkbox and search bar work well with each other. The complication you mentioned, still exist in current implementation. E.g. toggling the checkbox ignores the search bar values and shows results corresponding only to the checkboxes.

I propose that we replace all the checkboxes with buttons that append or replace the search bar value so that both work in coherent. E.g. [clear], [gradle], [java] etc and the lines summary can be moved to the search results which updates along with the search condition. E.g. All: 2741 (443) gradle: 14 (2) jade: 75 (9) java: 1438 (258) js: 477 (72) md: 129 (34) scss: 571 (60) yml: 37 (8).

Alternatively, instead of auto-updating the search bar, we can simply consider the combination of the two implicitly. i.e., if the documentation group is ticked and the user types *foo*.adoc in the search bar, we filter out files that meet both criteria.

The advantage of my proposition over this alternative is that:

  1. Easier to manage the url bookmarking. Only need to parse a single field.
  2. Eliminate race condition and concurrent actions. If we have both checkbox and search bar which operates on a single list of files concurrently, it can result in crash.
  3. Currently, there is no atomic way to show a specific file type. E.g. java. User would have to hide all files first, follow by selecting the java files which result in unnecessary waiting time to hide all files.
    With this idea, we can just click on java button and the search bar will be updated accordingly to show only java files in an atomic action. To hide all java files, we can probably implement middle mouse click to show the inverse.
  4. Lower complexity of implementation, and reduce redundant codes.
  5. LoC update with changes.
  6. In tandem with my suggestion in Custom grouping of files #547

@damithc , what do you think?

@damithc
Copy link
Collaborator

damithc commented Feb 9, 2019

@damithc , what do you think?

Good points.
I kind of dislike the idea of auto-updating a field that is also meant to be editable by the user. Users are unpredictable, for example, they can edit the auto-generated text in arbitrary ways.
In here, I'm proposing to keep tick boxes and filter box independent. i.e., only one will be used at any time. Would that reduce the complications?

@eugenepeh
Copy link
Member

I kind of dislike the idea of auto-updating a field that is also meant to be editable by the user. Users are unpredictable, for example, they can edit the auto-generated text in arbitrary ways.

I see. Though, another advantage of auto-generated text is that it helps user understand how the glob works .

In here, I'm proposing to keep tick boxes and filter box independent. i.e., only one will be used at any time. Would that reduce the complications?

It does helps to reduce the complexity, though not any helpful to the bookmarking because it is still 2 separate field. However, if users are more comfortable with the ui in #547 (comment), I am all in for it as my main concerns are mainly the complexity and potential bugs getting the 2 working together.

@damithc
Copy link
Collaborator

damithc commented Feb 9, 2019

I see. Though, another advantage of auto-generated text is that it helps user understand how the glob works .

We can show the auto-generated glob under the tick boxes as a read-only label so that the user can copy it over to the filter box, if he wants.

It does helps to reduce the complexity, though not any helpful to the bookmarking because it is still 2 separate field.

That's true. In this case, we'll have to encode filter text or tickbox configuration, whichever the chosen option. On a general note, it maybe good to define a data structure that can represent all report parameters, and functions to encode it into a URL (and decode it from the URL). perhaps something based on JSON?

However, if users are more comfortable with the ui in #547 (comment), I am all in for it as my main concerns are mainly the complexity and potential bugs getting the 2 working together.

Yes, getting the two to work together will be hard. Let's keep them separate.

Copy link
Contributor

@ongspxm ongspxm left a comment

Choose a reason for hiding this comment

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

minor nit

package.json Outdated Show resolved Hide resolved
@apoorva17
Copy link
Contributor Author

@damithc i've updated the implementation as per #547 (comment)

Copy link
Contributor

@yamidark yamidark left a comment

Choose a reason for hiding this comment

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

Do update the report image in the UserGuide as well, only DeveloperGuide updated

@apoorva17
Copy link
Contributor Author

apoorva17 commented Mar 2, 2019

Do update the report image in the UserGuide as well, only DeveloperGuide updated

Ok, I've updated it.

Copy link
Contributor

@ongspxm ongspxm left a comment

Choose a reason for hiding this comment

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

minor nit, the rest looks ready to go

},

getFilteredFiles() {
},
Copy link
Contributor

Choose a reason for hiding this comment

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

this is here because?

Copy link
Contributor Author

@apoorva17 apoorva17 Mar 3, 2019

Choose a reason for hiding this comment

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

I tried to do v-on:submit.prevent for glob filter to prevent the page from refreshing without specifying any method but it wasn't working so i had to add this method in.

Copy link
Contributor

Choose a reason for hiding this comment

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

sflr; pls add a comment here if needed. but try this first, it should work, using vanilla form functions:

form(onsubmit="return false")

@eugenepeh eugenepeh requested a review from damithc March 3, 2019 04:21
@damithc
Copy link
Collaborator

damithc commented Mar 3, 2019

On a separate note, is there a glob syntax to specify multiple conditions? e.g., having Test or Manager in the file name.
@apoorva17 a follow-up PR you can do is to support more complex conditions in the glob filter, such as OR AND

@apoorva17
Copy link
Contributor Author

apoorva17 commented Mar 3, 2019

On a separate note, is there a glob syntax to specify multiple conditions? e.g., having Test or Manager in the file name.

Not currently, I'm using minimatch library which does not seem to support such a glob syntax.

@yong24s
Copy link
Contributor

yong24s commented Mar 3, 2019

On a separate note, is there a glob syntax to specify multiple conditions? e.g., having Test or Manager in the file name.

Not currently, I'm using minimatch library which does not seem to support such a glob syntax.

Can try multimatch. It extends minimatch library to support multiple conditions.

@apoorva17
Copy link
Contributor Author

Can try multimatch. It extends minimatch library to support multiple conditions.

Sure, I can look into this in a follow-up PR.

@eugenepeh
Copy link
Member

@ongspxm just a reminder, you have an unresolved conversation #532 (review)

@eugenepeh eugenepeh requested a review from ongspxm March 5, 2019 13:44
Copy link
Contributor

@ongspxm ongspxm left a comment

Choose a reason for hiding this comment

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

minor nits

},

getFilteredFiles() {
},
Copy link
Contributor

Choose a reason for hiding this comment

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

sflr; pls add a comment here if needed. but try this first, it should work, using vanilla form functions:

form(onsubmit="return false")

@ongspxm
Copy link
Contributor

ongspxm commented Mar 5, 2019

@apoorva17 sry, i pressed wrong, dun know how to unapprove, but just that change will do

@apoorva17
Copy link
Contributor Author

@ongspxm I've made the requested change.

@apoorva17
Copy link
Contributor Author

@yong24s getting same frontend error like here

@yamidark
Copy link
Contributor

yamidark commented Mar 6, 2019

@apoorva17 Restarted it a few times and it passes now, seems like cypress will fail the tests once in awhile (happens to be locally as well). Will raise an issue for this.

@eugenepeh
Copy link
Member

Nits to proposed message:

The addition of #170 provides users with the convenience of filtering
the files in code view by formats using tick boxes. However, this does
not solve the problem of finding specific file(s) by name, folder or
path.

As a solution, let's add a search box to provide users with an
additional option of filtering files by glob patterns. E.g.
**/test/*.java.

@eugenepeh eugenepeh merged commit 5da9d45 into reposense:master Mar 6, 2019
yamidark pushed a commit that referenced this pull request Mar 22, 2019
After #532 , the name of the checkbox is changed from `mui-checkbox` to
`mui-checkbox--filetype`, causing an alignment problem with the
checkboxes.

Since this rename is meaningful, let's add another css part to ensure
that the checkboxes align.

Meanwhile, some of the indentation in #532 does not follow our
standard, let's update them as well.
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.

None yet

7 participants