miraisolutions / comparewith Goto Github PK

vignettes such as a user manual and a more detailed description of the functions need to be written

There is an inconsistency beteen compare_with_repo() and compare_with_other(), in that the compared file / directory is shown as "right" and "left" in Meld, respectively.

We should align the two, keeping the "right" behavior: you normally expect the working version on the right and the version to compare against on the left.

For example when the user doesn't have any file active in the editor but tries to run Compare with....

In general, we should capture all situations leading to any argument to compare_meld() being NULL.
In particular, the following errors should be captured with informative stop() messages:

• No active file in the editor (getSourceEditorContext()$path)
• No file selected (selectFile(...))
• No active RStudio project (getActiveProject())

From @vladdsm

I have started to add this functionality into my docker container stack https://github.com/vladdsm/docker-r-studio
Did not manage to let it work...

Instructions on how to install package when using RStudio docker to follow

In order for compareWith to launch Meld inside a containerized RStudio, we need to allow the graphical output from the Docker container to be displayed on the host machine graphical device.

Here is a working example on a Linux host machine (we use rocker/verse to leverage on many pre-installed packages, but the same works with any image from rocker/rstudio up in the Rocker stack).
We use the simplest approach based on mapping the DISPLAY environment variable and volume /tmp/.X11-unix when creating the container:

docker run -d -p 8787:8787 \
  -e USER=me -e PASSWORD=robust_password \
  -e DISPLAY=$DISPLAY \
  -v /tmp/.X11-unix:/tmp/.X11-unix:ro \
  --name rstudio_compareWith_test \
  rocker/verse:3.6.0

R and RStudio typically would not get the DISPLAY environment variable right, hence we need to add it to Renviron

docker exec rstudio_compareWith_test bash -c \
  'echo "DISPLAY=${DISPLAY}" >> /usr/local/lib/R/etc/Renviron'

Then we install Meld by running

docker exec rstudio_compareWith_test bash -c \
  'apt-get update && apt-get install -y --no-install-recommends meld'

It should then be possible to install compareWith inside RStudio via

remotes::install_github("miraisolutions/compareWith")

and use the addins. This can be tested by checking out a project from GitHub, e.g. usethis::create_from_github("miraisolutions/compareWith").

There will most likely be many dbus, GLib and dconf warnings / errors displayed at the R console when running Meld, this is annoying but should not be problematic.

The example above uses the simplest approach to graphical output from containers. See this Rocker
issue, this SO answer and this article for some context and alternative, more advanced approaches, especially when user rights and security concerns might be an obstacle to the simplest one.

Cleanup

    docker rm $(docker stop rstudio_compareWith_test)

The documentation of Addins functions was reviewed in 63641e7, going towards a per-Addin documentation compared to the initial design of documenting all Addins in the same @rdname, which is kind of a sensible thing but maybe was not so clear. This should be re-discussed and a general review of roxygen tags should be done (the insisted use of @description and @rdname is pretty un-conventional).

Caused by

This is currently not possible due to r-lib/actions#419

README needs to include more in-depth instructions regarding installation of Meld for Mac, also in view of the investigations in #3 .

Assess the desirable degree of modularization and re-factor accordingly

Thank you for integrating Meld with RStudio! I noticed a couple of related possible issues:

1. It takes a long time to launch Meld after selecting "Compare with Repo" from the addins menu in RStudio.
2. Launching Meld from the addins menu rapidly opens and closes dozens of git.exe terminals, effectively preventing the user from doing anything else while Meld opens.

I think these are related, hence submitting a single issue. Is this expected (perhaps OS-specific) behavior? Is it Meld's behavior, and not specific to {compareWith}?

R version 4.0.4 (2021-02-15)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 19042)

Matrix products: default

locale:
[1] LC_COLLATE=English_United States.1252 
[2] LC_CTYPE=English_United States.1252   
[3] LC_MONETARY=English_United States.1252
[4] LC_NUMERIC=C                          
[5] LC_TIME=English_United States.1252    

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods  
[7] base     

loaded via a namespace (and not attached):
 [1] Rcpp_1.0.6            compiler_4.0.4       
 [3] pillar_1.6.0          sys_3.4              
 [5] tools_4.0.4           packrat_0.6.0        
 [7] jsonlite_1.7.2        memoise_2.0.0        
 [9] lifecycle_1.0.0       tibble_3.1.0         
[11] pkgconfig_2.0.3       rlang_0.4.10         
[13] reprex_1.0.0          DBI_1.1.1            
[15] rstudioapi_0.13       curl_4.3             
[17] fastmap_1.1.0         dplyr_1.0.5          
[19] httr_1.4.2            stringr_1.4.0        
[21] xml2_1.3.2            generics_0.1.0       
[23] fs_1.5.0              vctrs_0.3.7          
[25] tidyselect_1.1.0      glue_1.4.2           
[27] R6_2.5.0              fansi_0.4.2          
[29] purrr_0.3.4           selectr_0.4-2        
[31] magrittr_2.0.1        ellipsis_0.3.1       
[33] assertthat_0.2.1      rvest_1.0.0          
[35] V8_3.4.0              rdocsyntax_0.4.1.9000
[37] utf8_1.2.1            compareWith_0.0.1    
[39] stringi_1.5.3         cachem_1.0.4         
[41] crayon_1.4.1         

The RStudio Addins menu can easily get very long, so we should try to limit the number of addins.

The existing "Compare with..." / "Compare with neighbor..." only differ by the starting folder of the file browser being the directory / file location. Do we really need both of them? I think it is fine to have a single addin "Compare with...", using the location of the current file as a starting point, from which it is easy to navigate e.g. up to the project directory.

• Agree on the addins
  • Keep only "Compare with other...", with same behavior as former "Compare with neighbor..."
• Agree on the order and names of the addins (shown in the RStudio Addins menu)
  1. Compare with repo
  2. Compare with repo - project
  3. Compare with other...
• Re-create the animated GIF in the README (#9)
  • See c10b0c5 and f89f916

As suggested and initially implemented by @statnmap in PR #37, we can use git difftool -t meld to compare with and between arbitrary Git revisions.

Here follow some (opinionated) design concepts

• The functionality and addins should mention "Git" (e.g. "Compare with Git"), as the existing compareWith functionality is not Git-specific (e.g. compare_with_repo() would work in case of an SVN repo)
• We should probably refer to "Git revisions" in general, see git-diff and gitrevisions. "Commit", although used in the help of git diff(tool), might be confusing for the user who might think about SHA only (e.g. based on the experience with GitHub repositories).
• When comparing between git revisions, the "first" should play the role of "selected for comparison" and the second of "compare against", and as such be on the "right" and "left" (see also #45).
• At least for addins, Git comparison should apply to the active project explicitly: git diff(tool) by default operates on the whole repo, but we are typically interested in changes specific to the active project, which might be in a sub-directory of the full repo.
• We could offer comparison of the staged changes or un-staged changes: this is supported by git diff(tool) but not as a revision, we can perhaps define something custom here, or consider it as a future extension
• We can offer comparison for both active project and file, similar to the existing addins
• Some rethinking of default revisions for the addins (when prompting the user).
  • We should avoid master as it could be main (or vice-versa), and it is perhaps not the most common thing to compare against.
  • A sensible default for comparing between revisions could be the current branch (HEAD) against the corresponding remote (@{upstream} or @{push}): this can be valuable prior to pushing committed changes
  • HEAD is probably still the best default for comparing the working version against a revision, although redundant with "Compare with repo".

Proposal

New addins:

• "Compare with Git" / "Compare with Git - project": Compare active file / project with a Git revision (branch, commit, ...)
  • Default: HEAD
• "Compare Git revisions - project": Compare Git revision (branch, commit, ...) for the project
  • Default: HEAD vs. @upstream

Update yaml file to include functionalities for both mac and windows

On Windows, the VCS repository comparison does not work, due to Meld not detecting git / svn when launched via system2(..., wait = FALSE).

This is probably intrinsic in the behavior of system() / system2() and we cannot easily work around it. We also do not want to switch to wait = TRUE for windows.

A valid alternative seems to switch to package sys. A quick PoC of calling Meld via sys::exec_background("meld", ".") on Windows confirms VCS are detected.

We should replace system2() calls with equivalent sys functions everywhere in compareWith. Note that we have unit tests from #3 (comment) for the meld version check.

.onLoad() function to message the version of meld or warn about its absence if not installed.

See Load hooks, help(".onLoad") for general information about the .onLoad() function.

By convention, .onLoad() and friends are usually saved in a file called zzz.R
[source]

We need to check the behavior of status <- system2("meld", "--version") across platforms to make sure we have cover cases in a robust and consistent way. Three cases to assess (see also help("system2"))

A) Meld is installed and the version is printed to stdout => (status <- system2("meld", "--version"))
B) Meld is not installed => mimic as (status <- system2("me_ld", "--version"))
C) Meld is installed but somehow called with --version causes errors => mimic as (status <- system2("meld", "--ver_sion"))

If, as hinted below, we would use stdout = TRUE (and stderr = TRUE since the two cannot be really separated), the returned status is a character vector and the behavior would be

R Markdown check_meld_version.Rmd for testing (to be renamed as .Rmd only, w/o the extra .txt)

• Ubuntu => The nominal version of meld installed via apt (3.18.0) does not cause any issues. The only difference wrt Windows is that error messages in Case B are more generic => R Mardkown output:
  check_meld_version_ubuntu.pdf

• Windows => Meld 3.20.0 seems to have issues with many Gtk warnings in the stdout. Issues disappear using the Meld 3.18.3 installer (as hinted on Meld's website, we should probably mention this in the package README), and the behaviour is consistent with the table above => R Mardkown output: check_meld_version_win.zip

• macOS => Meld 3.19.0 crashes. Meld 3.16.0 works pretty smoothly => R Mardkown output:
  check_meld_mac.pdf

See comment to commit 7b53203#r33984145

I might be missing something, and I think it would have been nice to pair this code change with an update to the existing unit tests revealing the cases the original implementation would not cover.

Overall, the idea is to use with_addin_errors() similar to withCallingHandlers(), which internally does not need to eval(expr).

I would also rather go for {...} (see the unit tests again) instead of expression(...) (and only when needed).

The commit was in fact a quick fix to an issue causing the addin binding not to re-execute after the first usage. This is due to the value body and being (lazily) evaluated instead of being kept as expression. The fix should be reviewed to preserve the intended meaning and usage of with_addin_errors() as described above.

GabrielBB/xvfb-action has been deprecated in favor of coactions/setup-xvfb and is no longer maintained.

Note that the no-longer-maintained GabrielBB/xvfb-action still relies on node12, which is a problem as confirmed by a warning in our pipeline

The following actions uses node12 which is deprecated and will be forced to run on node16: GabrielBB/xvfb-action@v1. For more info: https://github.blog/changelog/2023-06-13-github-actions-all-actions-will-run-on-node16-instead-of-node12-by-default/

Release current development version on the main branch as v0.1.0

Use a dummy example of a project under Git with existing merge conflicts.

To be done after #8, which might change the list of addins.

Setup an RStudio project for a checkout with conflicts and only compareWith Addins

1. Follow the instructions in miscellaneous/foobar/README.md to create a local checkout with merge conflict.
2. Create an RStudio project on the foobar directory.
3. Create an .Rprofile defining a dedicated .libPath() as follows

# Dedicated library path where only compareWith should be installed to have a
# clean Addins menu
lib <- paste(Sys.getenv("R_LIBS_USER"), "compareWith", sep = "-")
dir.create(lib, showWarnings = FALSE)
message("Set library path to ", sQuote(lib))
.libPaths(lib)

4. Added as foobar/.Rprofile and foobar/foobar.Rproj to the .gitignore in the parent directory)
5. Restart the R session and install compareWith

install.packages("remotes")
remotes::install_github("miraisolutions/compareWith")

6. Restart the R session an check the Addins.
7. Open foo.R and bar.R, with bar.R as active document, showing the conflicts.

Steps for the animation

0. Rstudio with diff / conflicts in the Git pane.
1. Addins menu with mouse over "Compare with repo (project)".
2. Meld window showing the list of blue / red files, and mouse over a red conflicted file.
3. Three-way comparison of the conflicted file.
4. Three-way comparison with resolved conflict and closing popup saying "Mark conflicts as resolved". (Optional, might be hard if we want a continuous usage)

Detailed process

Required tools (Linux): peek, gifsicle, convert.

Record the animation (the main challenge is to have straight mouse paths, which require practicing to get used to where things are located).
0. Copy na.rm = TRUE to the clipboard.

1. Open RStudio and set the default light color scheme, set the panes geometry as in the existing GIFman/figures/compareWith-RStudioAddins.gif, select the conflicted "bar.R" in the Git pane.
2. Run the "Compare with repo (project)" and set the Meld window size and position as in the existing GIF, leave it open, we will not re-launch it during recording since the position will be lost.
3. Launch Peek, and set the "delay before start" preference to 10 s. Then resize the capture window to fit the RStudio window excluding the title.
4. Start recording, and in the 10s go to Meld first (important for switching back to Meld later) and then RStudio, position the mouse on the "bar.R" in the Git Pane and wait for the count-down to finish.
5. Go to and click the Addins menu, mousover Compare with repo (project) and wait for the tooltip to appear, then quickly press Esc and Alt+Tab. This will bring you back to the previously-open Meld, giving the impression you just launched it.
6. Double-click to bar.R in Meld to show the 3-way diff/merge with the conflicts
7. Click on the right-to-middle arrows for each of the 2 conflicted chunks
8. In the middle file, click and Ctrl+V between the parentheses of the foo_sum() and foo_prod() calls to enter na.rm = TRUE.
9. Press Ctrl+S and click on the x in the foo.R tab to close, then click "Mark as Resoved" in the prompt.
10. Press Ctrl+Altr+R to stop recording, save the file as compareWith-raw.gif

Post-process the animation (remove unwanted frames at the beginning / end, add progressive blurring)

1. Open the console and cd to the location of the saved file.
2. Create directory mkdir frames and cd frames
3. Extract all frames gifsicle -U -E ../compareWith-raw.gif
4. Check initial and final frames if any should be excluded (e.g. due to the Peek countdown being displayed)
5. Given the last frame, e.g. compareWith-raw.gif.126, create blurred copies via

for i in {1..8}; do convert compareWith-raw.gif.126 -blur 0x$i zzz-blur.$i; done

6. Recreate the full GIF gifsicle -O3 * -o ../compareWith-RStudioAddins.gif

The DESCRIPTION file should be cleaned-up and made minimal.