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

diff editor: bundle a new :builtin-web GUI diff editor #3292

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ilyagr
Copy link
Collaborator

@ilyagr ilyagr commented Mar 14, 2024

When called with --tool :builtin-web, jj will start a local web server and
open a web browser with a GUI to edit the diff. See
https://github.com/ilyagr/diffedit3 for more details (or to run it as a
standalone executable). This is the diff editor previously advertised in
#3094, with some improvements since.

I would like to bundle it with jj so that everybody has access to a GUI diff
editor that can be run locally or over SSH (with port forwarding). It is
intended for situations where it is a fuss (or impossible) for a user to install
Meld and use the meld-3 configuration.

Some TODOs and thoughts:

  • This diff editor is somewhat restricted; it will ignore symlinks and currently
    has no support for executable bits for example. There are some known bugs, see
    e.g. the end of the `diffedit3 README. I'm hoping it's already usable.

  • Bundling the diff editor seems to add ~1.5MB to the jj binary. This is more
    than I expected. Unless there's a way to optimize this, I'm thinking of making
    the diff editor an optional feature, but I'd like it enabled by default, at
    least in release binaries. Or we could not worry about it.

  • I'm considering folding the diffedit3 repo (or the majority of it) into the
    jj source repo, so that it benefits from Dependabot, established code review
    procedures, and the reviewers we have for jj. The downside is that jj will
    then contain some Typescript code. However, there will be no need to install
    node or npm unless you are specifically working on the webapp; the
    "compiled" webapp is included in the repo.

  • Currently, :builtin-web works just like an external diff editor, creating a
    temporary directory on disk and then modifying it. At some point, we might want
    to switch to keeping the tree in-memory.


Here is a screenshot of v0.1.2 of the diff editor:

screenshot for diffedit3 v0.1.2

The PR is a bit unfinished and messier than I'd like. Some TODOs:

  • docs and changelog
  • allow configuring the port used by the local server and whether to automatically open a browser. In addition to a config, I'm considering adding a :builtin-web-noopen tool that's just like :builtin-web, but never opens a browser.
  • I'm considering renaming the current builtin.rs to builtin-tui.rs, as well as extracting builtin-web.rs and moving the common functionality between external.rs and builtin-web.rs to either mod.rs or a new file.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@ilyagr ilyagr changed the title diff editor: bundle a new :builtin-web GUI diff editor diff editor: bundle a new :builtin-web GUI diff editor (draft, RFC) Mar 14, 2024
@ilyagr ilyagr force-pushed the diffedit3 branch 4 times, most recently from 834a74f to b6cae90 Compare March 15, 2024 01:08
ilyagr added a commit to ilyagr/jj that referenced this pull request Mar 15, 2024
…TRUCTIONS

This will be reused for integration with the new `:builtin-web` diff editor in martinvonz#3292.

`instructions-path_to_cleanup` is moved into DiffWorkingCopies.

DiffWorkingCopies: add instructions_path_to_cleanup
ilyagr added a commit to ilyagr/jj that referenced this pull request Mar 15, 2024
This is preparation for martinvonz#3292, which will use these functions. The main
goal is to merge the parts of martinvonz#3292 that are likely to cause merge
conflicts with other PRs while I polish it up.
ilyagr added a commit that referenced this pull request Mar 15, 2024
…TRUCTIONS

This will be reused for integration with the new `:builtin-web` diff editor in #3292.

`instructions-path_to_cleanup` is moved into DiffWorkingCopies.

DiffWorkingCopies: add instructions_path_to_cleanup
ilyagr added a commit that referenced this pull request Mar 15, 2024
This is preparation for #3292, which will use these functions. The main
goal is to merge the parts of #3292 that are likely to cause merge
conflicts with other PRs while I polish it up.
@ilyagr
Copy link
Collaborator Author

ilyagr commented Mar 15, 2024

There was some discussion with @yuja (#3296 (review) and the following comments) about whetherdiffedit3 should be bundled in the jj binary. I'm thinking of asking on Discord what people think, but I think there should be a reference from this PR as well.

@thoughtpolice
Copy link
Collaborator

thoughtpolice commented Mar 15, 2024

I'll be taking this for a spin this week as I'm now running this locally; just wanted to leave one comment and say thanks for all this! diffedit3 really does look fantastic. I'll give an actual review later. Regarding one point...

  • I'm considering folding the diffedit3 repo (or the majority of it) into the
    jj source repo, so that it benefits from Dependabot, established code review
    procedures, and the reviewers we have for jj. The downside is that jj will
    then contain some Typescript code. However, there will be no need to install
    node or npm unless you are specifically working on the webapp; the
    "compiled" webapp is included in the repo.

See also #1997, where I wrote this:

A more concrete example? A theoretical jj web that I keep talking about on Discord. We might have Typescript code that uses React that depends on HTML and hell, maybe all that TypeScript also depends on Rust compiled to WebAssembly. The sky is literally the limit in the webdev world. And we need the Rust code to depend on it, as a distributable. We need to run linters, formatters, and run test suites on that code just like we do with Rust, today to the same standards and quality. And all that can be achieved with a unified UX that behaves the same way. How do you build the HTML frontend? buck2 build. Testing? buck2 test. Running the server with every change? buck2 run.

Today thought I don't think it's a huge deal, but once we start adding more server-side components with UIs, or maybe something like gg to the codebase, well... Maybe in the far future, if we actually went and boiled the oceans, we could ameliorate this exact issue.

@ilyagr ilyagr force-pushed the diffedit3 branch 6 times, most recently from c8d7eb1 to 259fb46 Compare March 20, 2024 01:34
@ilyagr ilyagr force-pushed the diffedit3 branch 4 times, most recently from 6c3451c to 86365ca Compare March 26, 2024 04:31
@ilyagr ilyagr force-pushed the diffedit3 branch 3 times, most recently from c1839b1 to 5a5ec8d Compare April 5, 2024 06:41
@ilyagr
Copy link
Collaborator Author

ilyagr commented Apr 5, 2024

@yuja, I think I'll start polishing this PR up in the near future. In addition to all the advanced users who can't run Meld, I think it would be eventually helpful for new users who don't have anything set up. This PR will not have this diff editor in the shape where we could make it the default (so that the new users get it), but I hope to eventually get it into that shape.

I got the sense that your objections softened over time, but if that's not so, let me know.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Apr 5, 2024

Other plans for the future would be to render JJ-INSTRUCTIONS differently (or replace them entirely) and to allow editing the commit description.

@ilyagr ilyagr marked this pull request as ready for review April 5, 2024 23:33
@ilyagr ilyagr changed the title diff editor: bundle a new :builtin-web GUI diff editor (draft, RFC) diff editor: bundle a new :builtin-web GUI diff editor Apr 5, 2024
@martinvonz
Copy link
Owner

I think Yuya's objection is fair. If we don't link diffedit3 into the binary but we still want to recommend it to users, and maybe even make it the default merge tool, how would we do that? Is that just a matter of communicating that to the people maintaining packages for different platforms?

@ilyagr
Copy link
Collaborator Author

ilyagr commented Apr 6, 2024

(Update: To be clear, I also think Yuya's objection is fair. The downsides are real, even though I did my best to minimize them, I could only do so much. I just hope that the upsides are greater to many people. I'm guessing Yuya mostly uses a few computers that he fully controls and are set up "just so", and the target audience to :builtin-web is the opposite kind of user).

As I said in the other thread, I would like any new user to be able to run jj split and get the nice interface for diffedit, without installing anything. I would also like to integrate the diff editor into jj closer, e.g. so that JJ-INSTRUCTIONS show up as instructions instead of a file. So, this PR is still my preference. We can back it out before the next jj release if we decide it's not worth it.

I don't think there's a good way to make this the default without bundling it with jj, and the easiest way to do that is to compile it in. Relying on packagers is a headache, I don't want people on different systems to get differently behaving jjs. (Update: Or, rather, I don't want it to be harder to build a good version of jj than a worse one. If somebody consciously decides they want jj - diffedit, it's fine for them to do it).

That said, we could start with recommending this diff editor in the docs and see what happens.

I have a few worries about that plan, mainly that the diff editor will not get enough mindshare to be worth my time supporting. Of course, I can't be objective in deciding whether it's good enough to be worth many people's time trying it (as would happen if, say, it was the default).

@ilyagr
Copy link
Collaborator Author

ilyagr commented Apr 6, 2024

I think Martin and Yuya have read this, but I'll quote myself at length from the other thread, where I mentioned a few more things.


I think you've read this, but for others, I'll quote myself from the other thread:

My goal here is to have a 0-configuration GUI that can work anywhere jj does. That's the main reason I took the time to make a tool that works from a local server (so, it works anywhere that has a web browser and over SSH). I think that a 3-pane diff editing experience is something that allows people to use jj in a powerful way they wouldn't be able to otherwise, so it's a good advertising for jj as well.

Requiring people to find, install, and configure the tool before they could use it would defeat most of its purpose. Anybody who can use and is willing to install Meld can use that already. On a Mac, or over SSH (to another machine, a VM, etc), using Meld is difficult or impossible even for people who are willing to put some time into it, and they get a sub-optimal jj experience in my opinion. (Or, at least, I'd like to get more people hooked on 3-pane diff editing, which is hard to explain to a new person but seems powerful to me, and intuitive once you try it).

I don't know an easy way to bundle the executables together without requiring configuration; getting jj to find the executable for diffedit3 automatically seems hard. In fact, if I wanted to bundle another executable with jj, I'd probably use something like rust-embed to include it into the binary. I was also hoping that linking them together would decrease the total size (since they use a lot of the same dependencies), but this didn't happen. I'm not sure why, so perhaps it could improve later :) (or not).

We can have the diff editor be a Cargo feature, so people who want to save 1.5MB of binary size could compile jj without it. (Also, perhaps this could be the default for incremental/debug builds)

....

I was and am considering making it possible for jj call :builtin-web without having to materialize anything on disk, but that would take some extra work.

One way to think about it is as analogous to linking in a built-in pager on Windows. The difference is that there is no platform where a good enough diff editor is provided out of the box.

....

UIs tend to be complex, fast moving, brings more dependencies, more flavors (web vs traditional), etc.

Yeah, those are problems. I tried to keep this one as simple as possible, but there's only so much I could do.

And I personally don't find it's hard to set up third-party tools for my needs.

I've been working in a few environments (remote Linux, Chrome OS Linux, Mac OS), and Meld had different challenges on each one (have to use remote desktop on remote, problems with Wayland on Chrome OS, having to find and use https://gist.github.com/syneart/4a8724cd479d31f0f742f499f807dcb2 on Mac).

@ilyagr
Copy link
Collaborator Author

ilyagr commented Apr 6, 2024

Also, let me know if it would help to implement a cargo feature to disable linking with diffedit3. That would be straightforward, if we want it. So far, it seems like it wouldn't make any difference to anybody.

@yuja
Copy link
Collaborator

yuja commented Apr 6, 2024

Maybe what I don't understand is why people love single-binary executable so much.

The user will have to install jj anyway, so for me, it's not much overhead to install diffedit3 in addition, and set ui.merge-editor = "diffedit3". Perhaps, nix or some other package managers will help eliminate the configuration headache?

@ilyagr
Copy link
Collaborator Author

ilyagr commented Apr 6, 2024

I'm still thinking about @martinvonz 's suggestions:

If we don't link diffedit3 into the binary but we still want to recommend it to users, and maybe even make it the default merge tool, how would we do that?

I guess one way would be to make the default "Use meld-3 if it is installed and we have a graphical environment, use diffedit3 if that is installed, otherwise use scm-diff-editor". I am not sure whether it's desirable to have logic this complicated for the default, but this is what came to mind.

Overlapping with the next topic, this approach is actually easier before jj is packaged, since currently anybody who installs jj has to know how to put a program in their PATH. If jj is packaged but diffedit3 isn't, plenty of users would start having trouble.

Is that just a matter of communicating that to the people maintaining packages for different platforms?

I don't think there are many such people yet, except maybe for the Nix ecosystem. But that is not a great option for new people, since Nix itself is not trivial and feels invasive to install (it's actually pretty good at keeping itself separate from the rest of the system, but I wouldn't require it for anything). Also, this is not an option for Windows.

If the world consisted of Debian, we could make diffedit3 a recommended dependency, but I'd hate having to navigate each distro's world. Homebrew, Fedora, Arch, Nix, Windows (do people use Chocolatey now?) are all twisty little passages, all different. I've thought a few times about packaging something (e.g. jj) for Debian, but I couldn't get past all the old-school conventions, and tools, and policies I'd have to learn. I feel like somebody would have to at least check the package for each distro to find out whether diffedit3 works (assuming we thought that it's the default experience we want for our users).

Some of what I wrote above might be influenced Yuya's question as well, but I'm still thinking about whether I have anything more direct to say on that topic.


If we can't agree on anything more, I would probably make a PR that advertises diffedit3 in our docs (probably next to the description of meld-3). I have concerns about the long-term viability of such a plan, but they could be overly pessimistic (I'll save them for a possible other post), and there might be things we could learn in the short term.

ilyagr added a commit to ilyagr/jj that referenced this pull request Apr 12, 2024
…ditor

This is instead of martinvonz#3292, which would make
`diffedit3` built into `jj`. I am still hoping to eventually make that a reality
and to make `diffedit3` a default diff editor that would be installed everywhere
`jj` is, but this may not happen, and it wouldn't help to test `diffedit3`
first. Some examples of concerns (see also the discussion in that PR):

- It is only a guess on my part that this would make a good default. The editor
might not be polished enough, and most users are not used to 3-pane diff
editing. I think most users would like it if they tried it, but this might be
plain wrong.

- There are concerns about adding a heavyweight dependency on `jj`. While I
tried to make it as lightweight as possible, it still unavoidably includes a web
server.

- There may be ways to bundle `diffedit3` with `jj` without combining them in a
single binary.
ilyagr added a commit to ilyagr/jj that referenced this pull request Apr 12, 2024
…ditor

This is instead of martinvonz#3292, which would make
`diffedit3` built into `jj`. I still have some hope of eventually making
`diffedit3` into the default diff editor that is available without any
configuration, which probably requires building it into `jj`, but this may not
happen, and it wouldn't hurt to test `diffedit3` first. Some examples of
concerns (see also the discussion in that PR):

- It is only a guess on my part that this would make a good default. The editor
might not be polished enough, and most users are not used to 3-pane diff
editing. I think most users would like it if they tried it, but this might be
plain wrong.

- There are concerns about adding a heavyweight dependency on `jj`. While I
tried to make it as lightweight as possible, it still unavoidably includes a web
server.

- There may be ways to bundle `diffedit3` with `jj` without combining them in a
single binary.
ilyagr added a commit to ilyagr/jj that referenced this pull request Apr 12, 2024
…ditor

This is instead of martinvonz#3292, which would make
`diffedit3` built into `jj`. I still have some hope of eventually making
`diffedit3` into the default diff editor that is available without any
configuration, which probably requires building it into `jj`, but this may not
happen, and it wouldn't hurt to test `diffedit3` first. Some examples of
concerns (see also the discussion in that PR):

- It is only a guess on my part that this would make a good default. The editor
might not be polished enough, and most users are not used to 3-pane diff
editing. I think most users would like it if they tried it, but this might be
plain wrong.

- There are concerns about adding a heavyweight dependency on `jj`. While I
tried to make it as lightweight as possible, it still unavoidably includes a web
server.

- There may be ways to bundle `diffedit3` with `jj` without combining them in a
single binary.
@ilyagr ilyagr marked this pull request as draft April 12, 2024 00:08
@ilyagr
Copy link
Collaborator Author

ilyagr commented Apr 12, 2024

If we can't agree on anything more, I would probably make a PR that advertises diffedit3 in our docs

This is now #3492


To finish up the discussion, my main long-term concern about #3492 is simply that not enough users will try 3-pane editing or go through the hurdle of installation to make diffedit3 worth my time to support. However, there's no harm in trying that approach first. Also, a friend of mine pointed out that if few people use it and I stop supporting it, that's far from the worst outcome.

My sense, however, is that people will benefit if 3-pane diff editing is given to them as the default. I think it's superior to git add -p and hg histedit-style diff editing, and people would be impressed by jj making it easy. OTOH, if I'm wrong, and this would be an imposition on people, or if the webapp is less useful to people than a TUI, that would of course be a problem.

@martinvonz
Copy link
Owner

I don't think there are many such people yet, except maybe for the Nix ecosystem. But that is not a great option for new people, since Nix itself is not trivial and feels invasive to install (it's actually pretty good at keeping itself separate from the rest of the system, but I wouldn't require it for anything). Also, this is not an option for Windows.

I agree that it would be good to have a default choice of graphical diff editor that works on all platforms. I also agree that it seems complicated to have to communicate that to all packagers. If we make diffedit3 a separate package, I suppose a related question is whether we should consider it a required dependency. IIRC, at least Debian packages can also have recommended dependencies. I don't remember what that means in practice (will you get a prompt when you install it?).

I'm fine with bundling it for now. That makes it easier for people to learn about it and to get feedback on the tool and on the workflow. Once it becomes popular enough that it's easy to install (platform packages available), we can stop bundling it. Or, if we decide after trying it for a while that it was bad idea for some reason, we can also stop bundling it.

Any other opinions?

@ilyagr ilyagr force-pushed the diffedit3 branch 2 times, most recently from 5b9101e to 711ca41 Compare April 17, 2024 00:38
ilyagr added a commit to ilyagr/jj that referenced this pull request Apr 17, 2024
…ditor

This is instead of martinvonz#3292, which would make
`diffedit3` built into `jj`. I still have some hope of eventually making
`diffedit3` into the default diff editor that is available without any
configuration, which probably requires building it into `jj`, but this may not
happen, and it wouldn't hurt to test `diffedit3` first. Some examples of
concerns (see also the discussion in that PR):

- It is only a guess on my part that this would make a good default. The editor
might not be polished enough, and most users are not used to 3-pane diff
editing. I think most users would like it if they tried it, but this might be
plain wrong.

- There are concerns about adding a heavyweight dependency on `jj`. While I
tried to make it as lightweight as possible, it still unavoidably includes a web
server.

- There may be ways to bundle `diffedit3` with `jj` without combining them in a
single binary.
@ilyagr
Copy link
Collaborator Author

ilyagr commented Apr 20, 2024

I'm fine with bundling it for now. That makes it easier for people to learn about it and to get feedback on the tool and on the workflow. Once it becomes popular enough that it's easy to install (platform packages available), we can stop bundling it. Or, if we decide after trying it for a while that it was bad idea for some reason, we can also stop bundling it.

I think we can go with #3492 for a month or so and see how that goes. I would still prefer to ultimately bundle diffedit3, but there is no immediate rush. I'm hoping that in a month we should have a stronger consensus opinion on what to do. We might get some sense that diffedit3 works for people, or that it does not, or how it can be improved. Even if we get none of these, I have no reason to think we'll be worse off for having waited.

@ilyagr ilyagr force-pushed the diffedit3 branch 2 times, most recently from cb75324 to a385651 Compare May 6, 2024 01:46
ilyagr added 2 commits May 5, 2024 19:58
When called with `--tool :builtin-web`, `jj` will start a local web server and
open a web browser with a GUI to edit the diff. See
https://github.com/ilyagr/diffedit3 for more details (or to run it as a
standalone executable). This is the diff editor previously advertised in
martinvonz#3094, based on CodeMirror5, with
some improvements since.

I would like to bundle it with `jj` so that everybody has access to a GUI diff
editor that can be run locally or over SSH (with port forwarding). It is
intended for situations where it is a fuss (or impossible) for a user to install
Meld and use the `meld-3` configuration.

Some TODOs and thoughts:

- This diff editor is somewhat restricted; it will ignore symlinks and currently
has no support for executable bits for example. There are some known bugs, see
e.g. the end of [the `diffedit3
README](https://github.com/ilyagr/diffedit3/?tab=readme-ov-file#shorter-term-todos-and-known-bugs).
I'm hoping it's already usable.

- Bundling the diff editor seems to add ~1.5MB to the `jj` binary. This is more
than I expected. Unless there's a way to optimize this, I'm thinking of making
the diff editor an optional feature, but I'd like it enabled by default, at
least in release binaries. Or we could not worry about it.

- I'm considering folding the `diffedit3` repo (or the majority of it) into the
jj source repo, so that it benefits from Dependabot, established code review
procedures, and the reviewers we have for `jj`. The downside is that `jj` will
then contain some Typescript code. However, there will be no need to install
`node` or `npm` *unless* you are specifically working on the webapp; the
"compiled" webapp is included in the repo.

- Currently, `:builtin-web` works just like an external diff editor, creating a
temporary directory on disk and then modifying it. At some point, we might want
to switch to keeping the tree in-memory.
ilyagr added a commit that referenced this pull request May 8, 2024
…ditor

This is instead of #3292, which would make
`diffedit3` built into `jj`. I still have some hope of eventually making
`diffedit3` into the default diff editor that is available without any
configuration, which probably requires building it into `jj`, but this may not
happen, and it wouldn't hurt to test `diffedit3` first. Some examples of
concerns (see also the discussion in that PR):

- It is only a guess on my part that this would make a good default. The editor
might not be polished enough, and most users are not used to 3-pane diff
editing. I think most users would like it if they tried it, but this might be
plain wrong.

- There are concerns about adding a heavyweight dependency on `jj`. While I
tried to make it as lightweight as possible, it still unavoidably includes a web
server.

- There may be ways to bundle `diffedit3` with `jj` without combining them in a
single binary.
@thoughtpolice
Copy link
Collaborator

thoughtpolice commented May 15, 2024

I'm in favor of including diffedit3, FWIW.

I think part of the reason goes back to Yuya's question which is that, people like single binaries simply because other tools are single binaries, too. It's not quite circular logic; rather there were tools that continuously adopted the single-binary idea, "one single file is all you need", and this has kind of hit an inflection point where many people consider it a major selling point. Really, it's just because it's easy and no hassle. Actually, on Linux, this same idea is why we ship statically linked -musl binaries, too. People want that because other people give it to them, and it's a single file that's guaranteed to work. (A funny twist of fate is when you end up optimizing binary size. People will ask for you to reduce binary size of the binary they asked you to make larger by including everything!)

IMO, I think diffedit3 is probably a good candidate to just be folded into the jj repo outright too, and we should just release the tools in tandem together in the same release tarball. It's not unheard of for a set of tools to be developed together like this, with one "leading the pack". So every downloaded release would come with jj, diffedit3, and who knows what else in the future (maybe a diffedit TUI?) This way users could hook it into other tools, too, while every build of jj is guaranteed to include it everywhere. I think that covers all the bases. We could do it separately, too. I think that's a smaller issue though.

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

Successfully merging this pull request may close these issues.

4 participants