Summary:
I made the split range selector overflow-x scroll so it would behave on small screens, but this broke the actual selection process, which is much more important.
I think I'll maybe redo how this looks/works in general so we don't have to have these vscode dropdowns, they're a bit wonky.
Reviewed By: quark-zju
Differential Revision: D49215034
fbshipit-source-id: 0fd3855753106e468287c2a86d98cbe47953ad78
Summary: if you drag to select lines, we can highlight them in the same way as if you're hovering on a line. This gives the strong hint that you can click any arrow to move all the lines together.
Reviewed By: quark-zju
Differential Revision: D49214070
fbshipit-source-id: ab76823b2b4b9a13c65a391b02e74e42b615a126
Summary:
The "files" tab in the edit stack modal is powerful, but it's not quite polished like split is right now. I propose we hide the files tab for the next release or so, until we can bring some of the changes from split into this view as well.
I believe this is a reasonable thing to do because the functionality provided here is not strictly different to what is provided by split—it's just a sort of UX optimization for a certain workflow.
I think we can make this into something very cool and useful, but I think we should focus on the split UI at first. If we leave this in in a less polished state, we risk diluting the value of the split UI and making it feel confusing as a whole.
Reviewed By: quark-zju
Differential Revision: D49213505
fbshipit-source-id: 92ec023b0ea2fd655de0b9ecff3b99425937f633
Summary:
- only show expand if some are collapsed
- only show collapse if some are expanded
Reviewed By: quark-zju
Differential Revision: D49211368
fbshipit-source-id: 8e0bedefded58631a3537e8e0d915790ee42e238
Summary: I noticed that context menus where the originator of the event is in the right half of the scren are offset incorrectly
Reviewed By: quark-zju
Differential Revision: D49211369
fbshipit-source-id: 2fd8587b12a1887a11ce4effeb6f777784e6880b
Summary: Add state to collapse files in split. This is done in the parent and passed to children so we can add "collapse all" / "expand all" later
Reviewed By: quark-zju
Differential Revision: D49210250
fbshipit-source-id: 96e9e5bad2c586bc66e1bfab1467eec9c75a5dc1
Summary:
Moving the range selector to the bottom adds a little extra vertical height we can use to render more code
I also think the typical workflow won't involve changing this much. I know it made sense to have this as the "first" thing you see, but since the state is saved regardless, you can change the range whenever without consequence.
Reviewed By: quark-zju
Differential Revision: D49209436
fbshipit-source-id: 47a27609e7c896468303668b29ab0a16c2c4ffea
Summary: small text and font tweak
Reviewed By: quark-zju
Differential Revision: D49204233
fbshipit-source-id: e23d777d1135ad0df3094a837e5e7f9b42d631da
Summary:
Add synatx highlighting to the split view.
Our existing highlighting infra is generally well set up for this.
However perf is a bit of an issue here. Two issues I had to solve:
- there's a lot more files potentially visible here than we'd normally expect in the comparison view, so trying to highlight them all at once is expensive
- to solve this, I set up an intersection observer so only visible files are syntax highlighted. Browsers are smart and this doesn't affect scroll performance as far as I can tell, but it will syntax highlight only as you scroll to new files which is great.
- not completely ideal yet is that we have to highlight the entire file if any part is visible, so long files don't get much optimization from this. However, it's still much better.
- we were converting from file text content into an array of lines on each render, and when that's passed into the highlighting hook, it would cause highlighting to happen each render, which caused new tokenization, which caused new highlighting etc in a loop.
- the fix is to memoize the Array<string> representation we extract from the text. Then we only highlight when the actual text changes (e.g. from clicking an arrow to move a line)
With these fixes, performance is bearable, but I won't say it's particularly great. If you click an arrow on a large file, it may take 40-60ms to move that line.
Some further optimizations we could do:
- don't highlight context lines, only visible _hunks_. This would save some processing for large files with small changes
- we could highlight in a webworker, so highlighting is completely async and doens't affect the main thread
- we rerun both before & after highlights when you click a line. Technically, this may not be necessary, we could skip half the work when you only click a single line's arrow
I'll also note that we don't allow editing right now. I have'nt explored how this plays with the editing mode.
We can also enable this for the "files" tab, but I haven't done so yet.
Reviewed By: quark-zju
Differential Revision: D49203833
fbshipit-source-id: 90d9088da422d3ac50d42738f0797be410c43ab9
Summary: Line selection regressed when changing to using a <table>, we can easily re-add this though. We should also deselect after moving lines, otherwise your selection persists weirdly.
Reviewed By: quark-zju
Differential Revision: D49200526
fbshipit-source-id: 75159ec6688ac02a14f994fc26469e122cd8a030
Summary:
Add a button between columns in the split view to add new blank commits. This allows you to decide you want finer granularity splitting midway through, and not have to manually shift everything over to make room for a new commit.
One TODO:
The way this works, it's creating "real" commits as opposed to the fake commit added to the end of the split. So if you make an empty commit and run split, it will actually make an empty commit.
I guess we should filter out empty commits when we go to apply the stack edit...?
What's more, it seems debugexportstack doesn't like empty commits, it fails to load split after creating some empty commits in my stack
Reviewed By: quark-zju
Differential Revision: D49193835
fbshipit-source-id: 787c21e8f23ec309efb9fee76e8e6294eb941ca3
Summary:
Some columns were bigger or smaller based on their content. Let's just fix them each at 700px or so. This is a random number, but it looks about right to me. We want each file to be large enough to render "normal" content without wrapping too much, while also for sure showing more than one column at a time on most screen resolutions.
We can't really guarantee the screen resolution. We could make this column size a function of the screen width, like enfore between: 300px < 70vw < 700px, so it has a min size, but then is based on screen, and then caps out at 700px. I didn't go for this because it can be a bit confusing if you resize and everything shifts around.
Reviewed By: quark-zju
Differential Revision: D49194521
fbshipit-source-id: e40c31cfa09d2ca36330e9e9e9b1c481d9997d42
Summary:
Add buttons in file headers to move the entire file left/right. This is equivalent to pressing each line in the file's left/right buttons.
I originally tried to do this with `fileStack.remapRevs`, but I kept running into it violating an invariant on the number of revs. Perhaps there's an easy fix for that, it would probably be a simpler implementation of this.
I'd love to get eyes on the logic here. I think it's reasonable, and mostly copied from what happens when you click a single line's arrow. But it is doing an extra check for context lines, which the single line implementation doesn't need to do (because it only runs on known changed lines).
Reviewed By: quark-zju
Differential Revision: D49172191
fbshipit-source-id: e6fa3ccfed4337fac8776fd5a36df873d5911ad6
Summary:
Use the split out function from the last diff in the SplitFile component. This lets us delete all the duplicated code.
We need to bring forward a couple of changes from split into this, but they're relatively harmless, mostly just tracking which line indices are added/deleted/context.
Reviewed By: quark-zju
Differential Revision: D49168989
fbshipit-source-id: 4203b2396a97daf135a4a2983fbdd110fca4f665
Summary:
In D49156730, we copied <FileStackEditor> into <SplitFile>. This is a lot of duplication of somewhat involved logic.
Now that we iterated on <SplitFile> a bit, it's clear that most of the code can be reused directly. So let's split out the reusable code that computes all lines, buttons, and line numbers.
Then it's up to the caller to choose how to render those lines. Either as one big <pre>, or a <table>, or compute the ribbons for split view, etc.
There's a TON of parameters we need to pass here, which feels a little silly. I think it's better to pass a ton of params to fascillitate reuse than to not reuse at all.
note: another approach would be to just leave this in one giant component that we code all behaviors into. I personally was finding it getting quite large and difficult to parse through. Extracting the line rendering makes it ever so slightly more readable if you ask me, so this is kind of worth doing anyway.
Reviewed By: quark-zju
Differential Revision: D49168992
fbshipit-source-id: 3e2dd03f131dcd99db0bb955be89f4b017c7e5ff
Summary:
picks up a bunch of changes Wilfred made to the hack grammar upstream
https://github.com/slackhq/vscode-hack/compare/v2.13.0...v2.17.1
Reviewed By: bolinfest
Differential Revision: D49194977
fbshipit-source-id: fb21ccd8c6642e9a567344c29fce92957dfafa18
Summary:
adds an option to format the textmate grammar files. this makes it possible to diff them.
i made it an option instead of always pretty printing them because i'm not sure how they're used everywhere. if they get minified again later we should just always do this.
Reviewed By: bolinfest
Differential Revision: D49195406
fbshipit-source-id: 65945ad7341bd2aa012699d0a04c5d945e5667dc
Summary:
`FilenameClassifier.getDisplayNameForLanguageId` takes a language ID like `hack` and is supposed to turn it into a display name like `Hack`.
first, it has to turn the language ID into a textmate scope name like `source.hack`, so that it can look up the `source.hack` textmate config.
but it was using `findScopeNameForAlias` instead of `findScopeNameForLanguage` to map the language ID to scope name.
we already have the language ID, not an alias, so the indirection through aliases is unnecessary -- and in Hack's case it's broken[1].
so this diff makes `getDisplayNameForLanguageId` first treat its arg as a language id, and fall back to the alias.
this is the same approach that highlighting uses:
https://www.internalfb.com/code/fbsource/[f8f30ba6a489]/xplat/vscode/vscode-extensions/code-compose/src/chat/TextMateMarkdownRenderer.ts?lines=156-160
[1] for some reason, the `ini` config seems to define an alias for Hack Configuration (ok) but also `hack`. this causes `findScopeNameForAlias('hack')` to return `'ini'`! the textmate grammar should be fixed, but that's a different bug.
Reviewed By: bolinfest
Differential Revision: D49191522
fbshipit-source-id: b6c0844dbf8ad2ea40ea3411037482f40db4679a
Summary: This gets rid of a hacky workaround that was written when the Windows hg launcher used the old hacky batch script. Thanks to quark-zju's stack from D49163002, these workarounds are no longer necessary and all integration tests on Windows can use the actual buck-built hg.
Reviewed By: quark-zju
Differential Revision: D49171182
fbshipit-source-id: 63ea7cabd1b93782bf3aba351a37087b655a57ba
Summary:
The "originalNodes" is important for setting up the mutation / predecessor
information. For the split case, it should be set to the commit being split.
This allows `max(successors(hashBeforeSplit))` to return the split stack top,
not the stack bottom.
Reviewed By: evangrayk
Differential Revision: D49081992
fbshipit-source-id: 555f44dde3339fb63a48638ae82cee3759624702
Summary:
The `:libhg` is gone, to use modules in `edenscm`, one
needs to register the special module finder/loader.
Reviewed By: zzl0
Differential Revision: D49178770
fbshipit-source-id: de17e1252225ebf84cedea3e9371a87dd40a3b81
Summary:
I'd like to differentiate the rebase operations trigerred by `amend`
and `rebase`.
For example, if a user deleted new added changes from an early commit of a
stack and then amend it. We cannot tell if the user wanted to delete the change
or move the change to a later commit.
Reviewed By: muirdm
Differential Revision: D49157532
fbshipit-source-id: 1221e1a0191cbd77f3cdd2b1f871443f3202621c
Summary:
Previously, we made commits with multi-line descriptions non-editable in split. I think it's more useful to let you change the title, and just keep whatever the existing description is.
In the future, we should probably also show you the description somehow (on hover?).
Here, I make the editing operation on the commit title field only affect the title, which gets split out for the editor.
Making every commit title editable is also more consistent for the UI, so it's not like, "why is only this commit's title not editable?"
Reviewed By: muirdm
Differential Revision: D49161482
fbshipit-source-id: 8157577c7d704cd7e44ec6948075ff294f0f920f
Summary:
Showing the blank right commit makes it more clear that there's a place that your changes will move to, and makes the mental model of split more clear on first load, that there's a set of commits laid out horizontally which you can move lines of code throughout.
This way, we also show `1/2` for the first commit by default, hinting that there are more commits that you'll be making.
Reviewed By: muirdm
Differential Revision: D49159468
fbshipit-source-id: 41880b29416cef7ad15a89d0638d7c8e19feed1d
Summary: Since we're not planning on enabling the edit file content button in the super near term, let's just remove this extra code. Same for the side-by-side editor view. We can show these features for the "files" tab instead.
Reviewed By: muirdm
Differential Revision: D49158968
fbshipit-source-id: f27773a482547b30c40b1ee4ae17655b21466ca7
Summary: Making the arrow larger makes it a bit easier to see and makes it more obvious that the entire point of the split ui is these little arrows
Reviewed By: muirdm
Differential Revision: D49157187
fbshipit-source-id: 4af76076e4a487d098e5245a8c8307717e299a60
Summary: Also tweak the "beta" thing to give some better spacing and use a red color
Reviewed By: muirdm
Differential Revision: D49156729
fbshipit-source-id: c38508ebaf00fff113c945538a1fd9e841985b8c
Summary:
Before, we used `' '` to ensure alignment. But now that we have a table, it's sufficient to have an empty `<td />`. So make the gutters null when they don't have an arrow in them.
This also fixes user selection to not include the gutters, since the `' '` was selectable, but now it's not
Reviewed By: muirdm
Differential Revision: D49156738
fbshipit-source-id: 5a4aed9388f3bbbd8122323573c8c807d4e514df
Summary: Add some darkening to the gutters so that the boundary between file content and line numbers
Reviewed By: muirdm
Differential Revision: D49156743
fbshipit-source-id: 7015e76d9d1901678ddc949f36281e9687004b3d
Summary: Tiny fix: without this, split wouldn't let you scroll all the way to the bottom
Reviewed By: muirdm
Differential Revision: D49156734
fbshipit-source-id: 950577052aa68a62a7bc470763de1dc3c55d0a74
Summary:
I was considering changing the ~~~~ context expander to match the comparison view, but I found that I didn't like how it looked. So let's keep the ~~~~. One small thing we can do is make it more clear that clicking will expand, by showing some text on hover.
Admittedly, by using CSS for this, it's not translatable.
Reviewed By: muirdm
Differential Revision: D49156741
fbshipit-source-id: d7229b439b721f72054b432cf8102da9d85c5ef3
Summary:
Since we re-used the file header from splitdiffview, we had an "open file" button next to each file. We don't want to take you out of the split commit UI, so we hide this button.
To do this, we can expose a new UI which lets you replace the open file button with arbitrary other content. We'll use this later to add left/right buttons for the entire file
Reviewed By: muirdm
Differential Revision: D49156739
fbshipit-source-id: 96e5899f06f65669bf6715e93736489cbc213a70
Summary: Small thing, it's nice to highlight the line you've hovering so you have higher confidence you're clicking the right left/right button. This is easy now that we have a <table>
Reviewed By: muirdm
Differential Revision: D49156736
fbshipit-source-id: 108966d60c127b55af9ef586d701c6122004e7c9
Summary:
Now that we render as a <table>, we can wrap lines properly within the table. As mentioned in the previous diff, this allows horizontal scrolling among your set of commits without trapping your horizontal scrolling "within" a single commit / file. This makes it much easier to move among a large stack of changes.
Additionally, wrapping is kind of nice because you can actually read long lines by default.
Reviewed By: muirdm
Differential Revision: D49156740
fbshipit-source-id: 2a3707e1bf355e28785a738c0756103d573bb9a3
Summary:
Change the entire rendering strategy to use a <table> instead of just a <pre>.
This has pros and cons, honestly.
Pros:
- Rendering is very straight forward, it's just a table
- you can do stuff like highlight the entire line on hover, since each row is its own thing
- this is how the comparison view works, and it works well
- each line can overflow wrap without messing up offsets. I believe this will be quite important because it allows you to horizontal scroll without having your scroll captured within a single file, which makes thes scroll experience a lot smoother when working with several commits horizontally. This is the primary motivation behind this change.
- syntax highlighting will be relatively easy to add
Cons:
- the single <pre> made content editing easier. Since we've opted to turn off content editing right now due to the confusing UX it entails, this doesn't immediately effect us. Of course, if we ever wanted to turn this back on, it's now a potential roadblock. I would argue that turning this feature back on will entail some UX changes. We can, for example, set the UI into a dedicated edit mode, where you have to explicitly confirm or cancel the inline edits, and at the moment you confirm, you also decide how the edit should be persisted through the rest of the stack (which was our UX confusion reason for disabling this feature). Given a scheme like that, using a <table> to render is not a blocker.
- selection/copy will now select multiple lines. I've already fixed this in the comparison view, and we can play similar games here. In fact, we can know during your copy event which lines are from which side of the comparison, so we could even skip removed lines and such if we wanted. Also, it's easy to just make the line numbers and buttons `user-select: none`, which automatically makes selection only include the normal contents as you'd expect.
Reviewed By: muirdm
Differential Revision: D49156732
fbshipit-source-id: c3b6c72bd2caa4b754eaaecfc0f7c4fd4a738fbd
Summary:
Let's invest more in the split view of the file stack editor. I'm splitting this out into its own copy, so we can iterate on it more easily and make large changes without necessarily affecting the other views in the stack editing flow.
This will let us do other kinds of rendering and fixes.
Reviewed By: muirdm
Differential Revision: D49156730
fbshipit-source-id: 213c810ce4a5ff6dbd139fe8efb1dfb14c5a0427
Summary:
The y-scroll behavior in the split view is a bit difficult to use with lots of files. Each file scrolls individually, which means to scroll down, you have to either be very careful where your mouse is, or do multiple scroll gestures / scroll very far.
Instead of limiting file height and overflow-y scrolling within them, we can just render all the file content directly. Even if these files are long, you can just more easily scroll among the code. Each split "column" will still have its own individual scroll, which allows you to scroll each column separately to compare content.
Now that we have this, we can reeanble the sticky header for files. This means you'll always be able to see the filename you're currently scrolling through, which provides context.
Reviewed By: muirdm
Differential Revision: D49156737
fbshipit-source-id: a54d47aaeb55e0bc9682a073d17c6aba4b581d87
Summary: If we can reuse the file header, we get a few features for free, like copying file paths, nicer rendering, and sticky scrolling
Reviewed By: muirdm
Differential Revision: D49156731
fbshipit-source-id: 28f3e38eeeea14065bad6ea6243a2e8dcc515651
Summary: Going to reuse the SplitDiffView file header. I realize now that it's inside ISL it doens't need anything from ctx, so we can simplify
Reviewed By: muirdm
Differential Revision: D49156733
fbshipit-source-id: 57df56526cf2cd9fa54a2b67797c4a4643a060b7
Summary: The existing arrows weren't rendering for me, seems they're not well supported. These ones look about the same and should be supported.
Reviewed By: muirdm
Differential Revision: D49156735
fbshipit-source-id: 66acf50bf594800b719ecbbab3ded721e8d0642c
