Fix tabs not changedTick instead of modified/ Don't show hidden bufs/scrollbar on overflow #626

cyansprite · 2017-08-17T03:50:25Z

If one would modify a buffer then undo it, instead of changing from
isDirty to not dirty (tabline with the little circle) it would remain
dirty because ticks update even when undoing...

Therefore check for modified instead.

This would have caused furthur issues whenever we implement
requesting not close on dirty.

Issue(s) : #624

If one would modify a buffer then undo it, instead of changing from isDirty to not dirty (tabline with the little circle) it would remain dirty because ticks update even when undoing... Therefore check for modified instead. This would have caused furthur issues whenever we implement requesting not close on dirty. Issue(s) : onivim#624

cyansprite · 2017-08-17T04:43:58Z

TextDocumentItem worries me because I'm not really sure where it comes from, I didn't get to look into it that much.

bryphe · 2017-08-18T13:31:38Z

browser/src/Plugins/Api/LanguageClient/LanguageClientHelpers.ts

    const text = lines.join(os.EOL)

    return {
        uri: wrapPathInFileUri(bufferFullPath),
        languageId: filetype,
-        version,
+        // modified,
+        version: 0,


Unfortunately this may cause problems. This is used in our client implementation of the "Language Server Protocol" (https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md).

The TextDocumentItem is used to notify the language server that the file has changed, and it should update its in-memory representation. This is important for stuff like autocomplete, enhanced syntax highlighting, etc.

Some implementations rely on the version being incremented, otherwise they won't bother re-parsing or doing work to update that in-memory version... this would then come up as issues during completion.

Oooooooh that's what that was when I was doing this I couldn't find where we made TextDocumentItem and it makes sense because it's language-server-protocol.

Okay yeah I have a plan to fix that.

bryphe · 2017-08-18T13:32:47Z

vim/core/oni-core-interop/plugin/init.vim

@@ -110,10 +110,7 @@ let context.windowTopLine = line("w0")
 let context.windowBottomLine = line("w$")
 let context.byte = line2byte(line(".")) + col(".")
 let context.filetype = eval("&filetype")
-
-if exists("b:last_change_tick")


Can we keep version here? The version seems like the right thing to send to the language servers, but &modified is definitely want we want for actually knowing if the buffer is dirty.

yeah, I didn't realize the thing about the text document item, it seems weird to me that they check the version and not if it's modified, but either way, I will fix it.

bryphe · 2017-08-18T13:33:34Z

browser/src/UI/Actions.ts

@@ -34,7 +34,7 @@ export interface IBufferUpdateAction {
    type: "BUFFER_UPDATE",
    payload: {
        id: number,
-        version: number,
+        modified: number,


We could change the modifieds here to boolean

(It might be more intuitive if it is a boolean - it would mean we'd need to convert it from a number to a boolean somewhere in the pipe - either before the action creator or in the action creator). If modified is a number, and I'm just glancing at it, it makes me think there might be multiple modified states or something - so making it a boolean would make this more crisp.

Yeah I was going to do that, but I left it as is because I know some vimmers are hard core numbers because c, but yeah I was thinking about changing it to bool too, so will do :).

bryphe · 2017-08-18T13:40:34Z

Thanks for jumping in and making the change, @cyansprite ! This is awesome, having the &modified flag for dirty tracking is a much better experience.

TextDocumentItem worries me because I'm not really sure where it comes from, I didn't get to look into it that much.

Yes, unfortunately I think if we remove the version there (or always send 0), it will cause problems, as the language servers use it and they might not update their version of the document. Which would then cause completion issues and stuff.

Can we send both the version and &modified for now?

The only other item I was thinking about was the modified field on the buffer objects in State.ts and Actions.ts - can we change that to a boolean? I believe the value would be only ever 0/1 so that would make the 'intent' of the field more clear.

Thanks again for putting this together! This is a huge improvement for the buffer line.

cyansprite · 2017-08-18T13:43:20Z

I agree with all your suggestions, I implemented this way for convenience and rapid prototyping, plus I was sleepy.

Fixing now

bryphe · 2017-08-18T13:52:58Z

I implemented this way for convenience and rapid prototyping, plus I was sleepy.

Haha, this is the same way I work too... Get it working in a fast / dirty way, then once it is working, I'll go back and take the time to clean it up.

cyansprite · 2017-08-18T13:55:31Z

So we use b:changeTicks, why don't we use

changenr()						*changenr()*
		Return the number of the most recent change.  This is the same
		number as what is displayed with |:undolist| and can be used
		with the |:undo| command.
		When a change was made it is the number of that change.  After
		redo it is the number of the redone change.  After undo it is
		one less than the number of the undone change.

This way the language server will only update IF we actually changed, not if we change then undo

bryphe · 2017-08-18T14:00:21Z

Interesting... I actually use b:changedTick because I saw that is what other completion providers were using. It seems like the difference is the 'undo' behavior - b:changedTick would always increase, whereas this would map to the change in the undolist. (I think YouCompleteMe is where I saw this)

I didn't know about changenr actually until just now - looking through the doc you posted, changenr would probably work OK too (and maybe even better, if the language service caches older versions). I haven't tested, but the only problem I could see is if there is a naive language service that only does the update if newVersion > oldVersion. But the most important scenario for this is the completion scenario - so as long as the version is incrementing as the user types - it seems like it would be fine.

cyansprite · 2017-08-18T14:15:55Z

so as long as the version is incrementing as the user types - it seems like it would be fine.

changenr doesn't do this on their own, it's not until they leave insert mode for the change to actually tick.
I see changetick actually does while in insert mode automatically.
We actually don't send buffer updates until on save is this the design we want?
Well for the syntax, completions of course work.

We can remedy changenr() by altering the undo time line with ctrl+g u, however hardcore vimmers might not like this, a mapping I use which I really like is

inoremap <CR> <C-]><C-G>u<CR>

that way I can undo each new line even after leaving insert mode, and I don't have to loose an entire paragraph or code block.

however, I don't think that's for everyone so changed tick would be better left untouched in this case. I think it's more hassle and just better to use changed tick, because then we would have to keep track or undo levels and changed tick, and then we would need to check what mode it's in to tell which we should check against.

bryphe · 2017-08-18T14:32:46Z

Thanks for the info / research!

We actually don't send buffer updates until on save is this the design we want?
Well for the syntax, completions of course work.

We actually send buffer updates on any change (this happens via an autocmd that hooks CursorMoved/CursorMovedI events and calls OniNotifyBufferUpdates) - but in insert mode we only send a subset (the current line). It turns out there is no 'perfect' event for listening to buffer changes - Neovim has an issue tracking adding a better event, though.

however, I don't think that's for everyone so changed tick would be better left untouched in this case. I think it's more hassle and just better to use changed tick, because then we would have to keep track or undo levels and changed tick, and then we would need to check what mode it's in to tell which we should check against.

Sounds reasonable to me. The latest changes look great!

cyansprite · 2017-08-19T04:04:32Z

Sounds reasonable to me. The latest changes look great!

Unless there is something else to add I think I'm done :).

cyansprite · 2017-08-19T04:38:29Z

Fixing #591 while I'm here since it's right in line with modified

cyansprite · 2017-08-19T05:30:33Z

Also small quick fix for #564
If user navigates to buffer that isn't scrolled to it doesn't focus this, going to see if there is an easy way to do this as well as the flex thing.

bryphe · 2017-08-19T14:03:48Z

browser/src/Editor/NeovimEditor.tsx

        } else if (eventName === "BufWritePost") {
-            // After save, there is always an additional change tick bump, so the `+1` is needed to account for that.
-            UI.Actions.bufferSave(evt.bufferNumber, evt.version + 1)


Glad we can lose the version + 1 hack here 👍

bryphe · 2017-08-19T14:12:01Z

Just looked through the latest changes - looks great to me! I'm really happy the version + 1 hack is gone, and that we now have a more robust way to know when the buffer is modified (and should be visible). I like the way you implemented this, too - it's a very natural extension of the state we have there today.

Looks like there are a couple of minor lint issues that the appveyor picked up (you can run npm run lint locally to reproduce them):

> oni@0.2.8 lint:browser C:\projects\oni
> tslint --project browser/tsconfig.json --config tslint.json && tslint vim/core/oni-plugin-typescript/src/**/*.ts
C:/projects/oni/browser/src/Plugins/Api/LanguageClient/LanguageClientHelpers.ts[110, 19]: Missing trailing comma
C:/projects/oni/browser/src/Plugins/Api/LanguageClient/LanguageClientHelpers.ts[126, 46]: Missing trailing comma

Once those are addressed, I'll merge this in.

Also, I put a small bounty on this issue as a thank you: https://www.bountysource.com/issues/48329508-tabline-says-dirty-after-undolevels-0

So make sure to claim that once this is in 😄

cyansprite · 2017-08-19T22:13:08Z

Thanks:) I will fix the errors when I get home.

Also, I put a small bounty on this issue as a thank you: https://www.bountysource.com/issues/48329508-tabline-says-dirty-after-undolevels-0
So make sure to claim that once this is in 😄

What's a bounty and it's not big deal I like to help and I love Oni:).

cyansprite · 2017-08-20T00:00:46Z

All fixed :)

bryphe · 2017-08-20T15:29:24Z

Just tested out the PR - the behavior feels so much better now, and the dirty mark actually behaves as I'd expect 😄 Great work, thank you for the contribution! I'll merge in now.

bryphe · 2017-08-20T15:46:13Z

What's a bounty and it's not big deal I like to help and I love Oni:).

I set up a bountysource page here for Oni and there have been some really generous donations, even in the early stage Oni is in! The funds are for Oni development, and as part of that, I think its important to show appreciation to contributors to the project. And I really appreciate the improvements you've made so far!

So once this issue is closed, make sure to "claim" the bounty here: https://www.bountysource.com/issues/48329508-tabline-says-dirty-after-undolevels-0

Thanks again for your contribution! 😄

cyansprite · 2017-08-20T16:00:20Z

Thanks again for your contribution! 😄

No problem!
And wow I had no idea this was a thing, this is amazing thank you!

bryphe reviewed Aug 18, 2017

View reviewed changes

Added back in version, change modified to boolean

e501fce

Don't show hidden/unlisted buffers in tabline

2010870

cyansprite changed the title ~~Fix tabs not changedTick instead of modified~~ Fix tabs not changedTick instead of modified/ Don't show hidden bufs Aug 19, 2017

Super simple scrollbar implementation for tabs

ae60001

cyansprite changed the title ~~Fix tabs not changedTick instead of modified/ Don't show hidden bufs~~ Fix tabs not changedTick instead of modified/ Don't show hidden bufs/scrollbar on overflow Aug 19, 2017

bryphe reviewed Aug 19, 2017

View reviewed changes

Fix linting

a9cc488

bryphe merged commit 81d664e into onivim:master Aug 20, 2017

cyansprite deleted the tabsModified branch August 20, 2017 16:00

bryphe mentioned this pull request Aug 24, 2017

closing tabs does not work #658

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tabs not changedTick instead of modified/ Don't show hidden bufs/scrollbar on overflow #626

Fix tabs not changedTick instead of modified/ Don't show hidden bufs/scrollbar on overflow #626

cyansprite commented Aug 17, 2017

cyansprite commented Aug 17, 2017 •

edited

Loading

bryphe Aug 18, 2017

cyansprite Aug 18, 2017

bryphe Aug 18, 2017

cyansprite Aug 18, 2017

bryphe Aug 18, 2017

bryphe Aug 18, 2017

cyansprite Aug 18, 2017

bryphe commented Aug 18, 2017

cyansprite commented Aug 18, 2017

bryphe commented Aug 18, 2017

cyansprite commented Aug 18, 2017 •

edited

Loading

bryphe commented Aug 18, 2017

cyansprite commented Aug 18, 2017 •

edited

Loading

bryphe commented Aug 18, 2017

cyansprite commented Aug 19, 2017

cyansprite commented Aug 19, 2017

cyansprite commented Aug 19, 2017 •

edited

Loading

bryphe Aug 19, 2017

bryphe commented Aug 19, 2017

cyansprite commented Aug 19, 2017 •

edited

Loading

cyansprite commented Aug 20, 2017

bryphe commented Aug 20, 2017

bryphe commented Aug 20, 2017

cyansprite commented Aug 20, 2017

Fix tabs not changedTick instead of modified/ Don't show hidden bufs/scrollbar on overflow #626

Fix tabs not changedTick instead of modified/ Don't show hidden bufs/scrollbar on overflow #626

Conversation

cyansprite commented Aug 17, 2017

cyansprite commented Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe commented Aug 18, 2017

cyansprite commented Aug 18, 2017

bryphe commented Aug 18, 2017

cyansprite commented Aug 18, 2017 • edited Loading

bryphe commented Aug 18, 2017

cyansprite commented Aug 18, 2017 • edited Loading

bryphe commented Aug 18, 2017

cyansprite commented Aug 19, 2017

cyansprite commented Aug 19, 2017

cyansprite commented Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

bryphe commented Aug 19, 2017

cyansprite commented Aug 19, 2017 • edited Loading

cyansprite commented Aug 20, 2017

bryphe commented Aug 20, 2017

bryphe commented Aug 20, 2017

cyansprite commented Aug 20, 2017

cyansprite commented Aug 17, 2017 •

edited

Loading

cyansprite commented Aug 18, 2017 •

edited

Loading

cyansprite commented Aug 18, 2017 •

edited

Loading

cyansprite commented Aug 19, 2017 •

edited

Loading

cyansprite commented Aug 19, 2017 •

edited

Loading