-
Notifications
You must be signed in to change notification settings - Fork 324
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
base: main
Are you sure you want to change the base?
Conversation
:builtin-web
GUI diff editor:builtin-web
GUI diff editor (draft, RFC)
834a74f
to
b6cae90
Compare
…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
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.
…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
There was some discussion with @yuja (#3296 (review) and the following comments) about whether |
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!
See also #1997, where I wrote this:
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 |
c8d7eb1
to
259fb46
Compare
6c3451c
to
86365ca
Compare
c1839b1
to
5a5ec8d
Compare
@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. |
Other plans for the future would be to render JJ-INSTRUCTIONS differently (or replace them entirely) and to allow editing the commit description. |
:builtin-web
GUI diff editor (draft, RFC):builtin-web
GUI diff editor
I think Yuya's objection is fair. If we don't link |
(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 As I said in the other thread, I would like any new user to be able to run I don't think there's a good way to make this the default without bundling it with 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). |
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:
....
....
|
Also, let me know if it would help to implement a cargo feature to disable linking with |
Maybe what I don't understand is why people love single-binary executable so much. The user will have to install |
I'm still thinking about @martinvonz 's suggestions:
I guess one way would be to make the default "Use Overlapping with the next topic, this approach is actually easier before
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 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 |
…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.
…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.
…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.
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 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 |
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 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? |
5b9101e
to
711ca41
Compare
…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.
I think we can go with #3492 for a month or so and see how that goes. I would still prefer to ultimately bundle |
cb75324
to
a385651
Compare
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.
…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.
I'm in favor of including 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 IMO, I think |
When called with
--tool :builtin-web
,jj
will start a local web server andopen 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 diffeditor 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 morethan 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 thejj source repo, so that it benefits from Dependabot, established code review
procedures, and the reviewers we have for
jj
. The downside is thatjj
willthen contain some Typescript code. However, there will be no need to install
node
ornpm
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 atemporary 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:
The PR is a bit unfinished and messier than I'd like. Some TODOs:
:builtin-web-noopen
tool that's just like:builtin-web
, but never opens a browser.builtin.rs
tobuiltin-tui.rs
, as well as extractingbuiltin-web.rs
and moving the common functionality betweenexternal.rs
andbuiltin-web.rs
to eithermod.rs
or a new file.Checklist
If applicable:
CHANGELOG.md