RefactoringMiner icon indicating copy to clipboard operation
RefactoringMiner copied to clipboard

Manual assignment of pairs of files

Open koppor opened this issue 1 year ago • 12 comments

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);
+            }
+        }
+    }

koppor avatar Apr 11 '24 07:04 koppor

@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.

tsantalis avatar Apr 11 '24 20:04 tsantalis

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 avatar Apr 12 '24 06:04 koppor

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 avatar Apr 12 '24 07:04 koppor

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 avatar Apr 12 '24 07:04 koppor

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 avatar Apr 12 '24 07:04 koppor

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

koppor avatar Apr 12 '24 07:04 koppor

@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:

  • [x] Improve the highlighting of comments within moved code https://github.com/tsantalis/RefactoringMiner/issues/703#issuecomment-2051108649
  • [ ] Avoid multiple move tool-tips. Possibly replace it with a single tool-tip that shows the moved code diff https://github.com/tsantalis/RefactoringMiner/issues/703#issuecomment-2051128301
  • [x] Introduce connecting lines between left and right side https://github.com/tsantalis/RefactoringMiner/issues/703#issuecomment-2051136238
  • [x] Use a different colour pallette more appropriate for colorblind people https://github.com/tsantalis/RefactoringMiner/issues/703#issuecomment-2051155616
  • [ ] Use a global setting (cookie) to store the preferred diff editor of the user. This will simplify the UI by avoiding the presence of two diff buttons https://github.com/tsantalis/RefactoringMiner/issues/671#issuecomment-2051144148

We have highlighting instead of connecting lines.

tsantalis avatar Apr 12 '24 13:04 tsantalis

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...

koppor avatar Apr 24 '24 10:04 koppor

@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.

tsantalis avatar Apr 24 '24 13:04 tsantalis

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

koppor avatar Apr 24 '24 14:04 koppor

@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 avatar May 01 '24 20:05 tsantalis

@koppor I fixed the mapping of comments in PR https://github.com/JabRef/jabref/pull/11180 See issue https://github.com/tsantalis/RefactoringMiner/issues/703#issuecomment-2051108649

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

tsantalis avatar Oct 06 '24 23:10 tsantalis

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

tsantalis avatar Jan 09 '25 14:01 tsantalis

I would really like to raise my wish again. Example from: https://github.com/tsantalis/RefactoringMiner/issues/894

Image

I would like to see the diff between the two yellow-marked files - to see how they relate. That request could even trigger some backend functionality, re-evaluating a diff.

koppor avatar Apr 27 '25 21:04 koppor