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

Manual assignment of pairs of files #703

Open
koppor opened this issue Apr 11, 2024 · 13 comments
Open

Manual assignment of pairs of files #703

koppor opened this issue Apr 11, 2024 · 13 comments

Comments

@koppor
Copy link

koppor commented Apr 11, 2024

Context: https://github.com/JabRef/jabref/pull/11180/files

There was code moved from src/main/java/org/jabref/gui/frame/JabRefFrame.java to src/main/java/org/jabref/gui/frame/JabRefFrameViewModel.java. E.g., storeLastOpenedFiles was moved there. I want to see an AST diff there.

Maybe, the use case is that methods go from one file n > 1 other files, which seems to be unsupported currently.

For me, it would be OK, to have a list of candidate files where changes are in and that I select one of the file for viewing (and then getting the MonacoDiff)


src/main/java/org/jabref/gui/frame/JabRefFrame.java

-    void storeLastOpenedFiles(List<Path> filenames, Path focusedDatabase) {
-        if (prefs.getWorkspacePreferences().shouldOpenLastEdited()) {
-            // Here we store the names of all current files. If there is no current file, we remove any
-            // previously stored filename.
-            if (filenames.isEmpty()) {
-                prefs.getGuiPreferences().getLastFilesOpened().clear();
-            } else {
-                prefs.getGuiPreferences().setLastFilesOpened(filenames);
-                prefs.getGuiPreferences().setLastFocusedFile(focusedDatabase);
-            }
-        }
-    }

src/main/java/org/jabref/gui/frame/JabRefFrameViewModel.java

+    void storeLastOpenedFiles(List<Path> filenames, Path focusedDatabase) {
+        if (prefs.getWorkspacePreferences().shouldOpenLastEdited()) {
+            // Here we store the names of all current files. If there is no current file, we remove any
+            // previously stored filename.
+            if (filenames.isEmpty()) {
+                prefs.getGuiPreferences().getLastFilesOpened().clear();
+            } else {
+                prefs.getGuiPreferences().setLastFilesOpened(filenames);
+                prefs.getGuiPreferences().setLastFocusedFile(focusedDatabase);
+            }
+        }
+    }
@tsantalis
Copy link
Owner

@koppor

storeLastOpenedFiles() is shown as moved to JabRefFrameViewModel in the diff.

Screenshot from 2024-04-11 16-43-56

@pouryafard75
Maybe, we could have something like a tooltip/popup that shows the diff when you hover over the moved method.

@koppor
Copy link
Author

koppor commented Apr 12, 2024

Side comment (not sure if I should open a new issue)

I don't get the semantics of red and green. In the screenshot the red and green text seems to be the same, but one is marked red, the other one green.

grafik


Tracing information: The diff was created by issuing following command

docker run -p 6789:6789  -v c:\users\koppor\rmc:/diff tsantalis/refactoringminer diff --url https://github.com/JabRef/jabref/pull/11180

@koppor
Copy link
Author

koppor commented Apr 12, 2024

The hover currently shows "Moved to File: X" multiple times:

grafik

Please show it only once!

(If you are on it, file should be with lower letter. Maybe even moved to file: to have the text more passive)

@koppor
Copy link
Author

koppor commented Apr 12, 2024

I think, what would help me is to have some letters/icons between the line numbers and the text. For instance M for moved to another file.

@koppor
Copy link
Author

koppor commented Apr 12, 2024

Side comment: I miss the connecting lines of IntelliJ. It is so hard for me to correlate left and right - even if a click (somehow) relocates the other scroll pane.

grafik

@koppor
Copy link
Author

koppor commented Apr 12, 2024

Fun fact: IntelliJ does not recognize that diff. Thus, I can only show at other place how IntelliJ displays:

grafik

Note the more lighter colors and the better color contrast?

They use gray for deletion (maybe because of colorblind people)?

grafik

@tsantalis
Copy link
Owner

tsantalis commented Apr 12, 2024

@koppor @pouryafard75

Indeed, we need to do a lot to improve the UI look and feel. I agree with all your comments.

To summarize all points in the issue:

@koppor
Copy link
Author

koppor commented Apr 24, 2024

Additionally, when opening the editor, it should scroll to the first diff. Example: When diffing 974bf46 (#1442)., I need to scroll down.


The commit 974bf46 (#1442). is also a motivation for the arrows.

image


Oh, I don't see added code on the right side?

On GitHub, I see the added lines:

image

OK, all are green... I would like to have a "mix" of current RefactoringMiner and the GitHub diff...

@tsantalis
Copy link
Owner

@koppor

This is an interesting case in terms of diff accuracy.
On the left side, there are 11 arguments, while on the right side there are 13 arguments.

This possibly means that 2 arguments are newly added, but it's a bit hard to understand what is the correct diff.

L81 seems it moved to R93. Is this correct?

L89 is now R95. Is this correct?

What are the truly newly added arguments?
It seems some arguments were reordered, some added, some updated with different numbers.

@koppor
Copy link
Author

koppor commented Apr 24, 2024

This is an interesting case in terms of diff accuracy.

:-)

Therefore I was asking about arrows to check if the tool would match some intuition.

This possibly means that 2 arguments are newly added, but it's a bit hard to understand what is the correct diff.

Sure :) - I also forgot, what I added :)

L81 seems it moved to R93. Is this correct?

Yes.

L89 is now R95. Is this correct?

Yes. Fixed-test-case.

What are the truly newly added arguments? It seems some arguments were reordered,

No reordering of arguments. It should be updates only to fix the expected value at the beginning (first parameter).

  • right L82 fixes left L83
  • right L83 fixes left L84
  • right L86 fixes left L85 (consistency to right L85)
  • left L88 got lost somehow? (or could be now right L87)
  • right L95 fixes left L89

@tsantalis
Copy link
Owner

@koppor

I have great news. We implemented additional diffs for moved code.
This means that for a given file, there could be multiple diffs, if some methods or attributes have been moved to another class, or extracted to a new class.

For PR https://github.com/JabRef/jabref/pull/11180/files
The new diff is
JabRefFrame.javaJabRefFrameViewModel.java

@tsantalis
Copy link
Owner

@koppor
I fixed the mapping of comments in PR JabRef/jabref#11180
See issue #703 (comment)

Screenshot from 2024-10-06 19-18-38

@tsantalis
Copy link
Owner

@koppor
Happy New Year Oliver! Thank you for your valuable feedback.
I wish you all the best for 2025.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants