State changed as code fixing the issue is committed

State changed as build #2673 is successful

Thanks for the quick update! I updated OneDev and opened a large pull request and try to review it. I think it is still not optimal. Here are some observations:

1.) OneDev always opens the "all files" view which is pretty slow. The PR has 40 files now (displaying 37 of them), with lengthly changes within files and the browser tab is barely usable (Mac, Safari). It feels pretty sluggish until you are able to hit the "one by one" link. For example when the "all files" view is open and has been rendered, it takes 1-2 seconds for mouse over effects to get rendered. 2.) During review I wanted to switch to a specific file in a new browser tab because the current file under review called code in the other file. I noticed that the file list itself is gone in 7.1.7 and it was pretty annoying to open a specific file in the PR (manually open a new tab and then hit multiple times 'next' to find the correct file). 3.) A file list also helps to quickly get an overview about the files changed. This gives you a first impression how large a PR is, which packages have been touched, you might even think something like "Eh, why has this file being changed for that PR, that's weird". Without a file list it is also not possible to start reviewing on a specific file / package because sometimes you review files in a specific order which is not alphabetically. So without a file list I felt a bit lost in the beginning.

So some improvements could be: 1.) OneDev should automatically switch to the "one-by-one" mode if too many file changes have been detected. In my PR are 37 of 40 files visible and it feels too much, so maybe reducing the limit might be worth as well. 2.) Both modes "all files" / "one-by-one" should display a file list. In "all files" it can be used to scroll to a specific file and in "one-by-one" it can be used to jump to a specific file. In both modes it can be used to open a new browser tab for a specific file, so you can more easily view multiple, specific files. 3.) This is optional but might be a nice user experience: If we have a file list, then in the "all files" mode it might be worth thinking about having a sticky header containing a dropdown component to jump to a specific file. That way you can always access the file list, even when having scrolled down a lot. Instead of a sticky header using HTML anchors might also be possible. That means you have one anchor per file so you can scroll to that file and a "back to file list" link somewhere which scrolls back to the top file list. This would also be useable with browser back/forward buttons and can integrate nicely with computer mice having back/forward buttons. Github currently tests a file tree on the left side and each file has it's own anchor (for example https://github.com/gradle/gradle/pull/20687/files). If you click through some files in the tree, you can move back and forward using browser/mice buttons.

Personally I think the "all files" view is nice for very small pull requests because you can quickly go through them. Maybe up to 10 files or a total of XXXX changed lines (whichever comes first). For all other cases I prefer seeing files one by one to have a snappy UI and having a UI control to jump between any file in the PR quickly (and possibly open them in new browser tabs).

Thanks for the suggestion. Maybe adding a cookie to remember the "show all files" vs "one by one" is better, as switching mode dynamically may cause confusions.

Also the file list is already available. Just click on the filter field, and all changed files will be displayed organized in directories. You may drill down, or filter based on name patterns to review part of the change set.

Thanks for the suggestion. Maybe adding a cookie to remember the "show all files" vs "one by one" is better, as switching mode dynamically may cause confusions.

Or remove the "all files" view (even if it hurts very small pull requests a bit) and make the "one-by-one" view very good, given that it has best performance in all cases. That would also simplify your code and new features must not be applied to two views in possibly different ways.

I used Gerrit Code Review quite a bit and never had the feeling it was unpleasant to use. Even though they have two dedicated views (file list only / file diff) it was easy to use and to navigate. Probably because the UI is pretty fast and works well with browser back/forward. I usually had one tab open with the file list view and another tab open where I stepped through file diffs using prev/next. Using the first tab I could easily open any file if needed and use that file while reviewing in the second tab. Would have been nice to access the file list view from within the file diff view, but maybe they will provide it one day.

If you really want to keep both modes, then a cookie is a good start. But a setting which defines the default view would also be nice.

Also the file list is already available. Just click on the filter field, and all changed files will be displayed organized in directories. You may drill down, or filter based on name patterns to review part of the change set.

The filter input box isn't usable in the one-by-one view.

If I view File A and then use the filter box I have to click multiple times until I reach File B. I can not open File B in a new browser tab from the filter popup. If I select File B then the "one-by-one" view is empty (because I have File A active and have filtered for File B). So it can not be used to switch to a specific file nor open a specific file in a new browser tab. Well and of course you need to know the path to the file and have to click multiple times. In a large project you likely not recall the path, so a flat file list would be more usable I think.

The filter gives you an overview on what has been changed. You may select a specific folder or specify a certain file pattern (-**/*.html for instance to exclude all html templates recursively) to review changes you are interesting.

I am curious why you want to open File B while reviewing File A. Is it because some changes in File A make you believe that File B may also be affected, and you want to check that further? In this case, you can simply input File B in the filter (no need to drill into folder), and OneDev will show if that file is changed, and if yes, you can hit Enter to show File B, and then hit browser back button to go back to File A. You may also open a new tab to do this.

OneDev originally shows a flat change list, but our internal users find that it is often hard to find which sub system is affected, as different reviewer may only need to review interested files.

I am curious why you want to open File B while reviewing File A. Is it because some changes in File A make you believe that File B may also be affected, and you want to check that further?

Yes kind of. Usually when File A uses File B and either both files are new or File B has been changed (method added, changed). Then I want to know what exactly that method in File B does if method naming isn't perfect and/or I don't know that method at all. Or if that method handles null input correctly if File A suddenly can pass null into it. Things like that. Basically some quick cross file checks to better understand and verify changes in File A.

In this case, you can simply input File B in the filter (no need to drill into folder), and OneDev will show if that file is changed, and if yes, you can hit Enter to show File B, and then hit browser back button to go back to File A. You may also open a new tab to do this.

Didn't know I could enter the file name directly. That makes it a bit quicker but when I hit ENTER only the input box is filled. Then I have to hit ENTER again to actually apply the filter. Ok I can live with that. However when in "one-by-one" mode and applying a filter, then I only see a String showing the full path of the file I have entered in the filter box and the link "show all files". Now I have to click the "show all files" link again to actually see the file. So switching to a different file via the filter is a bit inconvenient in "one-by-one" mode. Maybe the UI should automatically and always be in "all files" mode when applying a filter? However that would also mean that when I manually remove/clear the filter I would expect to be in "one-by-one" mode again (with the correct file) since that is where I started (this would happen naturally when using back button in browser).

However with the above knowledge, I could open two tabs now, one in "all files" mode for using the filter box and one in "one-by-one" mode to actually review. HOWEVER given that the "all files" view only shows 37 of 40 files there are situation where the filter simply does not show any files. For example from these 40 files there is one .sql file. When I now type into the filter box -**/*.java .sql I can see the SQL file in the dropdown of the filter but applying the filter results in 0 results being shown in the file diff UI, because that sql file is one of the 3 files not being shown.

What I dislike using the filter input box approach during a review is that I have to switch between mouse and keyboard often. Maybe it is just a habit but it feels more comfortable if you can work through files using mouse only because you only read and sometimes write a comment. Coming from Gerrit it was very easy to do so. Clicking a link with the mouse wheel opens the link in a new tab, back/forward buttons on the mouse and switching tab using the mouse. So everything was doable with just reading code and navigating using the mouse. Now I have to switch do keyboard more often which feels more complicated.

OneDev originally shows a flat change list, but our internal users find that it is often hard to find which sub system is affected, as different reviewer may only need to review interested files.

I see. But if your internal users now use the filter box to achieve that, couldn't they have used the filter box before as well? I mean once I enter a filter, the flat list of files would be filtered as well, wouldn't they? So they would see a flat list of files they are interested in. Now that list is gone completely and you may see these files in the filter box.

Didn't know I could enter the file name directly. That makes it a bit quicker but when I hit ENTER only the input box is filled. Then I have to hit ENTER again to actually apply the filter. Ok I can live with that. However when in "one-by-one" mode and applying a filter, then I only see a String showing the full path of the file I have entered in the filter box and the link "show all files". Now I have to click the "show all files" link again to actually see the file. So switching to a different file via the filter is a bit inconvenient in "one-by-one" mode. Maybe the UI should automatically and always be in "all files" mode when applying a filter? However that would also mean that when I manually remove/clear the filter I would expect to be in "one-by-one" mode again (with the correct file) since that is where I started (this would happen naturally when using back button in browser).

You do not need to press ENTER twice, just input partial path of the file you want to open, and hit tab to complete it, followed by a ENTER to show it (check the hint at bottom of the filter dropdown). It is actually an auto-completion system helping you write patterns (the full path is treated as a pattern without wildcards). The nothing being displayed in one-by-one mode when re-filtering is a bug, it should reset to show first matching file in this case.

Alternatively you can also hover mouse over method in question in the diff view, and OneDev will prompt possible definitions of that method, so that you can go to the method definition directly.

However with the above knowledge, I could open two tabs now, one in "all files" mode for using the filter box and one in "one-by-one" mode to actually review. HOWEVER given that the "all files" view only shows 37 of 40 files there are situation where the filter simply does not show any files. For example from these 40 files there is one .sql file. When I now type into the filter box -**/*.java .sql I can see the SQL file in the dropdown of the filter but applying the filter results in 0 results being shown in the file diff UI, because that sql file is one of the 3 files not being shown.

The pattern -**/*.java .sql means that all files with exactly the name .sql not ending with .java. The correct syntax should be -**/*.java **/*.sql

What I dislike using the filter input box approach during a review is that I have to switch between mouse and keyboard often. Maybe it is just a habit but it feels more comfortable if you can work through files using mouse only because you only read and sometimes write a comment. Coming from Gerrit it was very easy to do so. Clicking a link with the mouse wheel opens the link in a new tab, back/forward buttons on the mouse and switching tap using the mouse. So everything was doable with just reading code and navigating using the mouse. Now I have to switch do keyboard more often which feels more complicated.

Most of the time, we click on the filter box, and it shows which top folders are changed, and then select the top folder and click the search button to show changes under that folder, or go down the folder to select relevant sub folders to show changes. Occasionally we input some negative patterns to filter off certain files, for instance html templates, css files etc.

I see. But if your internal users now use the filter box to achieve that, couldn't they have used the filter box before as well? I mean once I enter a filter, the flat list of files would be filtered as well, wouldn't they? So they would see a flat list of files they are interested in. Now that list is gone completely and you may see these files in the filter box.

Actually our users use the flat file list vary rarely, as it only serves the purpose of jumping between files. As mentioned earlier, our review workflow is normally select a subset of changes, and get them reviewed. Also the large change set not being fully shown issue can often be solved by reviewing a subset of changes each time. If we want to check some other file, we simply find that file with filter again.

Also even if I can filter the flat file list, it may not suffice our users, as it can not aggregate changes into a top down hierarchy to instantly get an idea of what subsystem is changed.

You do not need to press ENTER twice, just input partial path of the file you want to open, and hit tab to complete it, followed by a ENTER to show it (check the hint at bottom of the filter dropdown). It is actually an auto-completion system helping you write patterns (the full path is treated as a pattern without wildcards)

Oh right, that makes it a bit easier

Alternatively you can also hover mouse over method in question in the diff view, and OneDev will prompt possible definitions of that method, so that you can go to the method definition directly.

Hm yes, but OneDev then jumps into the commit UI and I loose information wether or not it has been added/changed in this PR (no diff coloring) and I should not write a code comment in that view because it is not attached to the pull request code comments. But it helps to see the implementation of the method. While testing it, I noticed a minor inconvenience in the popup showing method definitions: When clicking the link using the middle mouse button / mouse wheel then the browser still loads the new page in the same tab instead of opening a new tab. I assume the link has some javascript attached which does the page loading. Maybe check explicitly for the left mouse button.

The pattern -**/*.java .sql means that all files with exactly the name .sql not ending with .java. The correct syntax should be -**/*.java **/*.sql

Interesting. Then missing consistency has confused me. When I type -**/*.java .sql the filter box popup shows sql files, so I assume that these sql files would also show up in the UI once I apply that entered pattern. However it doesn't. If I type -**/*.java **/*.sql then the filter box popup does not show any sql files but after applying the filter the UI indeed does show the sql file. This can even be simplified: entering .java shows java files in the popup but applying it does return zero results. Entering **/*.java shows nothing in the popup but in the UI after applying the filter. So you basically get positive feedback for the pattern that does nothing. Confusing.

Shouldn't both act the same for consistency? Basically I thought the content of the filter box popup acts as a preview of what the UI would show once the pattern is applied. But that is currently not the case.

Most of the time, we click on the filter box, and it shows which top folders are changed, and then select the top folder and click the search button to show changes under that folder, or go down the folder to select relevant sub folders to show changes. Occasionally we input some negative patterns to filter off certain files, for instance html templates, css files etc.

Always interesting to see different approaches. After updating OneDev my coworkers directly asked where the file list can be seen 😃

Anyways after this discussion I would come up with the following improvements:

1. Reduce the limit of shown files to speed up the browser tab if many changes have been done in a PR. The current limit makes the tab barely usable when opening the PR the first time without any filter applied. I don't even want to think about mobile phones / tablets opening such a PR. I guess they will simply kill the tab directly because of memory constraints.
2. Fix the mentioned filter bug in "one-by-one" mode, so you can switch files in that mode.
3. Think about making the applied filter consistent to the filter box popup content so you can treat the popup content as a preview of what will happen when applying the currently entered filter.

Suggestions to think about as a compromise between your way of working and other people:

1. Add a setting "Always use one-by-one mode". When activated you show the flat file list below the filter box and no file diff at all when opening the PR.

• The filter box can filter the flat file list.
• One has to click a file to get to the file diff.
• The file diff view would be the same as it currently is showing prev/next links.
• This would work relatively similar to how Gerrit works

2. Instead of 1. rework the current "all files" mode

• Instead of showing all file diffs, render a file tree with empty folders being collapsed. Similar to how IDEs collapse empty Java packages. So when opening the PR there are no file diffs visible, only the file tree of changes. Basically show directly what is currently hidden inside the filter box.
• You would quickly see your subfolders / subsystems that have changes as you do now. But instead of seeing them in the filter you see them right in the file tree. If it helps a "collapse all" and "uncollapse all" icon button could be added so even folders with files will be collapsed to shorten the view if desired. That would make it even quicker to see your subsystems right away.
• When clicking a folder in the tree you go to a file diff view showing all file diffs in that folder (like in the current "all files" mode with applied filter). That would avoid typing because you already see the folder structure in the UI and only have to click on it.
• When clicking a file you go into the "one-by-one" view with next/prev links. this is useful if a folder has too many file changes and you have to go through them one-by-one.
• The filter then might only be used to exclude certain files in most cases.

These are just two ideas how to bring two different workflows closer together. I kind of like 2. but it is probably also the most work to get a prototype going to see how it feels.

Will improve the filter to get consistent experience.

As to display file list first in both suggestions, it complicates review process for major pull requests which are small (at least at our side) and file filtering is not necessary at all. Current mechanism is an evolvement of our own review process to make simple things simple, and complicate things possible.

Nevertheless, I will think about your ideas for next iteration of review workflow. Thanks for all the ideas, 👍

State changed as code fixing the issue is committed