model_diff: switch to diffviewer?

kyleam commented 1 year ago

@dpolhamus noted that model_diff displays inline rather than popping up in the viewer by default. Both @seth127 and I thought the viewer was the default and maybe there was an unintentional change. However, looking at our model_diff code and the diffobj package (model_diff uses diffobj::diffFile), that doesn't seem to be the case. You can get the viewer window by calling model_diff with .viewer = TRUE, but FALSE has been the default since it was introduced (9934717c, 2021-03-02).

I'd guess the reason that .viewer defaults to FALSE even for interactive use is that

you need to press enter to continue

https://github.com/metrumresearchgroup/bbr/blob/a46baec4935ed5b0af6387980dc5ef99b793c83a/R/model-diff.R#L41-L44
the inline diffs works with consoles outside of RStudio

We probably expected a different default due to other projects that use diffviewer::visual_diff().

Should we switch to that for doing the diffs? Based on quick testing, I think it works okay with knitting.

pro: assuming RStudio users prefer the viewer, better default without the "press enter" quirk
con: no option for inline diff. That's more than cosmetic because in interactive consoles outside of RStudio, visual_diff doesn't generate any output

@seth127, what do you think?

@andersone1, I know you've used diffviewer a fair amount on internal projects. Any thoughts on diffviewer vs diffobj?

seth127 commented 1 year ago

I'm confused by these two statements:

I think it works okay with knitting.

and

no option for inline diff. That's more than cosmetic because in interactive consoles outside of RStudio, visual_diff doesn't generate any output

What does it display when knit?

kyleam commented 1 year ago

Here's what I get with knitting a chunk with:

devtools::load_all()
m1 <- read_model("inst/model/nonmem/basic/1")
m2 <- copy_model_from(m1)
diffviewer::visual_diff(get_model_path(m1), get_model_path(m2))

html

```html ```

And here's what I see in a terminal R console when I run visual_diff:

> devtools::load_all()
ℹ Loading bbr
> m1 <- read_model("inst/model/nonmem/basic/1")
> m2 <- copy_model_from(m1)
> diffviewer::visual_diff(get_model_path(m1), get_model_path(m2))
>

kyleam commented 1 year ago

That of course depends on html output. So another con: it doesn't work with other output targets (note that the original pr, gh-342, explicitly considered docx and pdf output). model_diff currently has some code that warns about targeting non-html output, but I suppose we'd should switch that to an error if we move to diffviewer:

https://github.com/metrumresearchgroup/bbr/blob/a46baec4935ed5b0af6387980dc5ef99b793c83a/R/model-diff.R#L128-L133

kyleam commented 1 year ago

So another con: it doesn't work with other output targets.

My take, even with the just the initial con of not working for consoles outside of RStudio, is that we should stick with diffobj; it's not worth losing functionality to get a bit nicer viewer behavior that's on by default. We could of course support both diffobj and diffviewer, but I don't think it's worth the extra complexity.

But I wanted to open the issue to 1) get others' thoughts (I'm admittedly not a good person to assess the value of an RStudio-related feature) and 2) if we do stick with diffobj, have a record of why we prefer it over diffviewer (which, again, we use a fair amount outside of bbr).

andersone1 commented 1 year ago

@kyleam

We had to make this exact same decision with the review package.

We ended up going with diffobj::diffFile() over diffviewer::visual_diff().

The reasons included:

Neither seem to be actively developed, so that factor was neutral
diffobj::diffFile seems to be more flexible (based on the number of the parameters that can be tweaked)

These are defaults we use for parameters:

diffobj::diffFile(
target = .previous_revision_temp_file,
current = .current_revision_temp_file, 
color.mode = "rgb",
mode = "sidebyside",
tar.banner = .previous_revision_header,
cur.banner = .current_revision_header
)

It is nice diffobj::diffFile() allows the "banner" option
diffviewer::visual_diff is developed by r-lib, which is a bonus, but was not enough of a reason to switch to it

kyleam commented 1 year ago

@andersone1 Thanks. Hmm, so you don't even expose the interactive argument of diffFiles (like model_diff does through .viewer), and just let it fall through to the diffobj.interactive option (which I believe defaults to the result of interactive()).

I guess your review revamp hasn't been out in the wild very long (edit: or perhaps not really at all, given there hasn't been a new mpn snapshotyet), but anybody reporting about the whole "press enter to continue" thing being annoying (and surprising, if they're expecting diffviewer's behavior)?

andersone1 commented 1 year ago

@kyleam - No feedback on the "Press Enter" aspect yet.

One thing I was considering was saving the html diff file to a temp dir, and then opening it as a tab (rather than the RStudio viewer which is sometimes too small), and then side stepping that "Press Enter" step completely.

seth127 commented 1 year ago

This is great discussion here. My takes:

we should stick with diffobj; it's not worth losing functionality to get a bit nicer viewer behavior that's on by default.

I pretty much agree with that, so that's kind of the decision right there. That said...

review package

The functionality in review is for diffing the current version of the file with a previous (notably, a previously reviewed) version of the file. This is unlikely to be used in any non-interactive context. That doesn't mean it isn't work considering whether we should tighten that part up, but the core intended usage is definitely interactive.

Contrast that with bbr::model_diff() which is very much intended to be able to do things like knit into a report (probably not a formal report, but what we might call an "intermediate deliverable") or a demo document (i.e. here)

possible changes here

The default => viewer

I would be in favor of changing the default to model_diff(..., .viewer = interactive()), slightly tweaking the docs, and leave it at that. I don't feel strongly about this, and it may not be worth the tiny change, but I do think it would be better UX in most cases. And I can't think of anything that it would break.

Side note: The main thing we don't want is .viewer = TRUE being triggered when knitting an Rmd, because that hangs the whole thing because of the "press Enter" issue. I don't think this would cause that problem though. It may cause problems when interactively running Rmd chunks, but we would just need to check on that.

The press Enter thing

My personal opinion is that this is moderately annoying, but still better than seeing the printed inline diff. (Aside from the Rmd problem noted above.)

anybody reporting about the whole "press enter to continue" thing being annoying

We'll just have to pay attention on this, but ultimately, without any actual feedback at this point, we're just guessing on UX preferences.

andersone1 commented 1 year ago

Ah yes this is a very good distinction to point out @seth127. I like this option as well .viewer = interactive().

kyleam commented 1 year ago

I pretty much agree with that, so that's kind of the decision right there

Yep. So in terms of my original issue/confusion, we're set.

I would be in favor of changing the default to model_diff(..., .viewer = interactive()), slightly tweaking the docs, and leave it at that.

No strong objections from me. Extracted to gh-590.

metrumresearchgroup / bbr