tsantalis
Synchronized scrolling at diff view
I am viewing a diff:
If I scroll on the left, there is no scrolling on the right. I think, the scroll bars should be synchronized. - A more advanced thing could be that the left scroll bar is bound to the right one, but the right one is independent (and thus changes the used offset of the left one).
I think, the current diff is really "too far away" so that any refactorings can be determined... https://github.com/JabRef/jabref/pull/10957/files (I am currently reviewing and I think, there was really too much removed and added without keeping the old functionality)
@koppor
Did you actually move/extract some code from importDatabase to preferredCitationMethod and mainCffContentMethod?
To me it looks that the code inside preferredCitationMethod and mainCffContentMethod has been written from scratch.
The tool was not able to match code from the left side to the right side.
Also, the "deleted" code from the left side is written in Java Stream API logic, while the "added" code on the right side is written in more traditional for if logic.
@tsantalis Ah - and if there is no relation, then the scroll bars are not synchronized. OK for me... Then you can close this issue. Seems to be an edge case...
@tsantalis Why was that closed as completed? I was about to open an issue with the same title. I am currently reviewing https://github.com/JabRef/jabref/pull/11245. On GitHub at https://github.com/JabRef/jabref/pull/11245/files, I can just scroll through the diff using my MouseWheel. When using the MouseWheel at RefactoringMiner, it just scrolls one file, not both.
Maybe this will be fixed with https://github.com/tsantalis/RefactoringMiner/issues/703. However, I wanted to restate this. I am still trying to use RefactoringMiner as ReviewTool for GitHub PRs.
Should I collect requirements so that RefactoringMinor can be replacement for GitHub PR review or should I stay away?
Currently, I think, RefactoringMiner is close, but it could also be like opening a can of worms :)
Requirement 1: Mark files as viewed
Requirement 2: "Continuous Scrolling" through files.
I just want to jump to the next file without having to click back and forth. It is also OK if the next file is opened if I keep scrolling down with the mouse wheel (and the old file closed)
Requirement 3: Be able to post comments to lines on the GitHub PR.
Requirement 4: Be able to edit the file (a link opening the GitHub edit link is OK)
Thank you @koppor
I will re-open the issue. Thank you for your invaluable feedback. Indeed, we are trying to make something better than the diff currently offered by GitHub.
Please understand that we are an academic team with just myself and a few students, and we lack expertise in front-end and UI design.
Perhaps a better approach would be to create a browser extension that simply replaces the diff view within the GitHub UI, so that we don't have to re-implement the entire GitHub PR review environment.
Please understand that we are an academic team with just myself and a few students, and we lack expertise in front-end and UI design.
At least you do have students 😅.
You can use me for issue refinement for students. I have 10+ years experience with supervising. Excerpt: https://devdocs.jabref.org/teaching.html
Perhaps a better approach would be to create a browser extension that simply replaces the diff view within the GitHub UI, so that we don't have to re-implement the entire GitHub PR review environment.
I would object here. The hard part is IMHO the diff view you have. Integrating GitHub is just a bit of few lines of REST calls or GraphQL calls.
I can reserve time for a MWE if it helps.
@koppor We have a Beta version for "Continuous Scrolling". Please give it a try when you find some time. The docker image has been updated with the new feature. You will find a new button "Single Page View (Beta)" that opens the "Continuous Scrolling" UI. Your feedback is very appreciated.
Requirements 1, 3, 4 in https://github.com/tsantalis/RefactoringMiner/issues/672#issuecomment-2075100158 can be done by using the GitHub API and store the entered information directly on GitHub.
@tsantalis I tried the "Continuous Scrolling" at https://github.com/JabRef/jabref/pull/11476.
I like at GitHub that I can use my middle mouse wheel and scroll around the changes. Reason: Typically, the most important (or interesting change) is not the first file, but something "hidden" inbetween. Through scrolling and reading, I try to find that. From there, I navigate around the source. (Yeah, at some point, I will ask to jump between identifiers - similar to Sourcegraph 😅)
My wish: If mouse wheel is used and the inner editor cannot scroll, scroll the whole page. This should bring a nicer user experience.
Side note: I wish, there was a possibilit to scroll right - I tried with mouse-wheel-right, but did not work:
I see that the current implementation prevents to add empty lines left and right to offer a "synchronization" between the code left and right (as GitHub does)
However, it is hard to find matches in following diff:
Spontaneous answer:
Current perception of the feature: All diffs are the same height - and do not adjust to the content.
Maybe, this is hard to achieve as it would require major rework of the display -- GitHub adds empty lines to enable a sync between left and right.
If there were no scrollbars, the step to have the mousewheel working is surely closer.
@koppor Just a small side note to clarify the confusion: Synchronized scrolling is not going to be possible in the ASTDIFF domain, because as opposed to textual diff, move actions are also part of the ASTDiff, and due to the existence of moves, synchronized scrolling is just conceptually incorrect to be discussed.
@pouryafard75 I partially understand.
Nevertheless, I see that the tool can collapse regions;
Thus, the tool does more than just displaying from line 1.
A step towards synchronization is for me finding matching pairs. These can be IMHO: same source line - or matched diff (e.g., changed something, e.g., rename)
Lets enumerate this as pairs p1, p2, p3, ... - lets use .l and .r for left and right.
if p1.l is visible and p1.r is visible and user srolls down so that p1.l is invisible and p1.l is invisible, but p2.l gets visible, then at the right p2.r should be visible, too. The scroll down should be so far that a) p2.r is visible but b) the as least lines as possible are scrolled down at the right side. If possible, one line down at left should lead to a one line down at the right. If in the mode of that lines are matching.
Naively, I would really use the first match from the top:
If users scrolls one line up at the left, the right also goes up one line. Reason: There is a match found.
I am not talking about that it should be possible in 100% of the cases. I think, it can work in 30% to 60% of the cases maybe?
Without connecting lines or other helpers to guide me in the view, I cannot review fast. - While writing that, my decision driver gets clear to me: I want to increase my throughput in code reviews. We have in avarage 5 to 10 newcomers each month requesting code revieweing. i would like to give feedback quickly. That means: I want to identify the critical part of a PR fast to be able to give quick feedback. If there is no critical thing, I can review all with patience. If I need to start reveiwing thoroughly from the start, I cannot give timely feedback, because I take more time than my time budget allows me to do.
@koppor Thank you for your feedback.
-
Regarding the synchronized scrolling. In contrast to line-based text diff, AST diff supports moves. Think of the scenario that you extract a piece of code from a method in the beginning of the file and the extracted method is placed at the end of the file. In a line-based text diff, this change would be shown as deleted and added code. But in AST diff, this change is shown as a move in the AST. The same scenario could happen if the methods in a class get re-ordered.
So, the way we support the synchronization of the scroll bars is by clicking on a piece of code from the left or right side and automatically scrolling the other side. This way supports the move scenarios described above. If the click-to-synchronize-scrolls is not working so well for you, we can try to improve it. There is a known issue about how many clicks are needed to get this feature activated (1, 2, 3). Sometimes it is random.
-
About the fixed diff height, I totally agree with your comment. I also had the same concern. It is in our TODO list to adjust the height according to the context.
@pouryafard75 @koppor I tried PR https://github.com/JabRef/jabref/pull/11476 and the middle mouse scroll does not work some times.
I mean when you are inside the scroll area of diff, the outer scroll does not work. And this gives the impression of a dead/broken UI.
We must expand the individual diffs to the exact space they need, so that there is no inner scrolling. The same way GitHub does it.
This is top priority issue for making the continuous scrolling useful and a pleasant UI experience.
Oliver is right. In this particular PR, the UI experience is not good.
- If the click-to-synchronize-scrolls is not working so well for you, we can try to improve it.
I am using the tool a bit naively. Not knowing anything of its use by heart.
Double click on a change (marked by an arrow) "jumps" to line 65, however, line 60 is highlighted:
Expected result on doubleclick somewhere:
(I achieved it by a single click left and single click right)
I know that a change can range multipel lines; Following "hack" should cover most cases: If ciursor outside of current ast-matched-lines, move cursor to first line of ast. -- This keeps both exiting "selections" (the gray bars) but also moves to the matched element.
@koppor The continuous scrolling works if you place the cursor exactly on the gray vertical line between the left and right side of the diffs.
This way enables the outer scrolling non-stop. I assume this is the way you would like to use the continuous scrolling.
By placing the cursor on the left or right side, activates the scrolling within this specific area, and the outer scrolling is getting lost.
We will try our best to fix all these issues, so that the user experience is smooth.
@koppor The continuous scrolling works if you place the cursor exactly on the gray vertical line between the left and right side of the diffs.
Yeah, I found that. I can also use the scrollbar at the right side. - I was "just" stating, what I would expect as a naive user 😅.
By placing the cursor on the left or right side, activates the scrolling within this specific area, and the outer scrolling is getting lost.
I was adding: If the end of the window is reached, the outer scrolling should take over. -- This will be obsolete if the editor heights will be adjusted.
We will try our best to fix all these issues, so that the user experience is smooth.
Thank you so much for continuing working on that part of the tool. I know that UI programming is hard.
Another nice PR to try out the scrolling (and diff view functionality): https://github.com/langchain4j/langchain4j/pull/1433
I tried today with git rmd 0a33b343a3532855013411ca...e1a895bf9f4b6a3ebdc362735a7ec2d1e7b2c1b9 (context: https://github.com/JabRef/jabref/pull/11430/; I wanted to review the changes the contributor did after my PR comments).
Would still be nice if the heights of the windows could be adjusted somehow. I am aware, this is difficult, because this synchronized scrolling is hard to implement 😅. I just wanted to state that always changing the mouse position for scrolling takes so much energy from me...
@koppor We are already working on a new version without IFrames.
This one will be very similar to the GitHub commit page. I believe @pouryafard75 is very close to complete this new feature.
That's a relatively easy PR to review. It has only additions :) Soon you will have what you are looking for.
Hi @koppor
The new Single Page View is ready.
@pouryafard75 dedicated a lot of time on its implementation.
Each file-level AST diff appears in its own div.
The div height is automatically adjusted to the diff contents.
There is a single scrolling throughout the page, and you can scroll anywhere on the page.
Expanding the hidden code, adjusts automatically the div height, as needed.
He have some ideas on how to link code from the left/right side that do not appear next to each other (side by side).
For the next weeks, we will be working on some journal paper revisions, so we will take a pause from working on this feature. We are looking forward to your feedback. A new docker image push is on the way, currently running in the Actions.
P.S. Please give it some time (a few seconds), until it fully loads, before you start scrolling. I tested it for all PRs included in this issue, and it works very smoothly.
@pouryafard75 In the single page view, in the cases of moved code between files, the displayed diff is not fully minimized.
Case study: https://github.com/spring-projects/spring-framework/commit/ad2e0d45875651d9a707b514dd3966fa81a9048c
org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/support/HttpEntityMethodProcessor.java -> org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/support/AbstractMessageConverterMethodProcessor.java
org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/support/RequestResponseBodyMethodProcessor.java -> org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/support/AbstractMessageConverterMethodProcessor.java
Single Page View Screenshot (does not trim the deleted/added code before):
Individual Page Screenshot (trims the deleted/added code before):
Challenging PR to test: https://github.com/JabRef/jabref/pull/11542
Not sure about the collapsing here:
When investigating https://github.com/JabRef/jabref/pull/12310, I still have the wish that the code views enlarge so that three is no scrolling at initial page load.
There is also a scroll bar missing inside the box - either increase size (which is my prio 1) - or add a scrollbar so that one knows how much code is there.
@pouryafard75 The single page view has the bug you fixed already (horizontal scrollbar appearing for no reason). Please fix it asap.
Sorry @koppor This is an unwanted side-effect from the recent diffBot action we introduced.
@koppor
Your PRs keep challenging our tool.
You have some Stream API migrations that our tool missed in PdfMergeMetadataImporter.java