These are chat archives for ensime/ensime-atom

9th
Feb 2016
Matthew de Detrich
@mdedetrich
Feb 09 2016 01:05
@hedefalk I have to say congrats, this is the first release of ensime-atom that seems to work without any problems
I am no longer getting false errors on imports, or other things
Hmm wait, I spoke too soon, I just did some coding and now ensime atom reports all imports as errors
@hedefalk Is there anything I can do to help report this? I have had this problem for a while
Jeff Wilde
@jeffwilde
Feb 09 2016 01:47
@mdedetrich, I think there are a few people who have seen this behaviour (myself included). The thing is, the display of those warnings is actually pretty dumb: it’s just reading the messages directly off the wire from ensime-server, so I’m not sure there’s much to debug in the actual error-marking end of things.
Matthew de Detrich
@mdedetrich
Feb 09 2016 01:48
Well its a false positive, however I don’t know how ensime/atom handles those things. Like it was working fine, and then some compile error (as I was testing it out) then triggered all of the import problems again
Jeff Wilde
@jeffwilde
Feb 09 2016 01:48
Not to say it’s simply ensime-server’s fault or anything, though. It’s possible it’s due to some complexity in the configuration, protocol handling, or race conditions when talking to ensime-server.
Matthew de Detrich
@mdedetrich
Feb 09 2016 01:49
@jeffwilde So what is the issue exactly?
Jeff Wilde
@jeffwilde
Feb 09 2016 01:49
I’ve seen the exact behaviour you describe.
Matthew de Detrich
@mdedetrich
Feb 09 2016 01:50
Yeah it has been around for a while, its one of the reasons why I don’t use atom ensime normally, but I keep coming back to check on its progress
Jeff Wilde
@jeffwilde
Feb 09 2016 01:50
I don’t know what the root cause is, unfortunately. I do know that receiving the messages from ensime-server and translating them for display is a simple-enough that it is unlikely to be a bug in that path.
do you have experience with the other ensime clients?
Matthew de Detrich
@mdedetrich
Feb 09 2016 01:51
In other words, its not that ensime-server is producing incorrect error messages, its due to how ensime handles things
Jeff Wilde
@jeffwilde
Feb 09 2016 01:52
I do think that there are incorrect messages coming off the wire from ensime-server. It would bre quite a stretch for ensime-atom to be making up all those messages itself, given the simplicity of the data path.
Matthew de Detrich
@mdedetrich
Feb 09 2016 01:53
Well it may be an issue with the client asking somethings that incorrect to ensime-server, however I will try it with Sublime and see if I have the same problem
Jeff Wilde
@jeffwilde
Feb 09 2016 01:53
I’m not saying it’s necessarily a bug in ensime-server, though, it could easily be caused by sending the wrong stuff into ensime server.
Matthew de Detrich
@mdedetrich
Feb 09 2016 01:54
Yeah precisely
Or it could be an issue with the protocol (swank vs jerk)
Jeff Wilde
@jeffwilde
Feb 09 2016 01:58
yeah, my intuition points to either screwing up what we’re asking for (I don’t actually understand what we’re sending into the server yet), or it’s some flakeyness with the old protocol.
I think it would be very helpful to play around with the same project in the other clients like you’re thinking. If you see the same problem in any of those, then we’ve caught ensime-server red-handed, if it’s not reproducable, then that’s a strong indication we’re doing something weird while managing our inputs to the server.
Matthew de Detrich
@mdedetrich
Feb 09 2016 03:38
@jeffwilde Just managed to get sublime up and running, its working fine without problems
So yeah, its an issue with Atom (or how Atom does stuff)
Viktor Hedefalk
@hedefalk
Feb 09 2016 06:05

This is interesting. It would be super-cool if someone would help me with getting full session logs on traffic between client and server for a "working client" compared to atom to understand what the difference is. I STILL think after @fommil 's info about contents vs contentsIn that this might be the issue. However, some false positives remain. The issue is that ensime-atom sends file content over the wire while at least emacs writes it out to file and just sends the file path to ensime-server. This I haven't fixed yet, but might be able later today.

One thing I HAVE fixed is that I noticed that ensime-atom unnescessarily always sent a typecheckBuffer request just before asking for implicits. This I put under conditions (only if buffer is edited). I think that the whole typecheckBuffer with contents over the wire is what making atom provoking weird server behaviour.

I'm going to put out a release now so that you can test if you get rid of the false positives on imports. I can't seem to get them anymore locally :)

v0.34.0 out, get it while it's hot! I'll write release notes later if @d6y doesn't beat me to it :)
Matthew de Detrich
@mdedetrich
Feb 09 2016 06:15
@hedefalk trying it now
Viktor Hedefalk
@hedefalk
Feb 09 2016 06:15
Since there were no wild protests I went ahead with the linter-api so our homebrew error panel is killed of in this release. @jeffwilde I first just killed off the great styling you did since but then the red squigglies went gone so I re-instated them. But maybe there is some cleanup there we can do after removing the manual error thingy: https://github.com/ensime/ensime-atom/blob/v0.34.0/styles/ensime.atom-text-editor.less. You gotta help me with that :)
@mdedetrich To be fair, remember to clean out .ensime_cache!
Matthew de Detrich
@mdedetrich
Feb 09 2016 06:17
Yup, I do that every time
Viktor Hedefalk
@hedefalk
Feb 09 2016 06:17
:)
Maybe we should auto-do that on releases too…
Put a client version-tag in a file under .ensime_cache and flush it on diff.
Matthew de Detrich
@mdedetrich
Feb 09 2016 06:17
@hedefalk Nope just happened again
Viktor Hedefalk
@hedefalk
Feb 09 2016 06:17
Crap.
Matthew de Detrich
@mdedetrich
Feb 09 2016 06:17
It occurs when I just save the file, let me confirm
ill clean .ensime-cache again
@hedefalk Yeah its still happening, however I know when the errors come up, its when you save the file (Command + S)
if you don’t save the file, then Atom works as expected
Viktor Hedefalk
@hedefalk
Feb 09 2016 06:23
OK. Good to know. Does it happen on the test project you published earlier?
Matthew de Detrich
@mdedetrich
Feb 09 2016 06:23
What test project?
Viktor Hedefalk
@hedefalk
Feb 09 2016 06:24
Hm, no, sorry, that wasn't you…
Matthew de Detrich
@mdedetrich
Feb 09 2016 06:25
Yeah, saving definitely causes the problem. Maybe check what you send to ensime-server when you save the file. That is what might be causing problems
Viktor Hedefalk
@hedefalk
Feb 09 2016 06:25
I know we ask for that a lot, but repro always helps.
Yes, I think so to. But I think I removed it in this release. I did some extra implicit stuff and sent the contents over the wire but I removed that. What typechecking settings do you have, on save or while typing?
Matthew de Detrich
@mdedetrich
Feb 09 2016 06:26
How do I view my typecheck settings, also this is company code so I can’t really publish it
Viktor Hedefalk
@hedefalk
Feb 09 2016 06:26
But there might me more paths to "typecheckBufferWithContentsOverTheWire" that I just missed so I'm going to get rid of it alltogether. Just need to decide where to put temp files.
Yeah, I get it. No worries, I've seen a lot of false positives on imports too so it's just a matter of time I guess before I get some myself again.
Settings is under settings -> packages -> ensime
cmd-, on mac
Just got it myself now!
Matthew de Detrich
@mdedetrich
Feb 09 2016 06:31
Viktor Hedefalk
@hedefalk
Feb 09 2016 06:31
Ok, I got lots of false positives when opening ANOTHER file in a bigger project. I'm going to fix this today, be confident :)
Matthew de Detrich
@mdedetrich
Feb 09 2016 06:31
:thumbsup:
Not a huge rush on my end
Viktor Hedefalk
@hedefalk
Feb 09 2016 06:32
Oh, change that "typing" to "on save" if you don't want the typechecker to get every keystroke over the wire.
That might improve stuff.
Matthew de Detrich
@mdedetrich
Feb 09 2016 06:32
Well I do, I am a lot less productive otherwise
Viktor Hedefalk
@hedefalk
Feb 09 2016 06:32
Ok, scrap that then, I'm going to change it to communication using files instead.
Matthew de Detrich
@mdedetrich
Feb 09 2016 06:32
(I need typechecking on every keystroke)
Viktor Hedefalk
@hedefalk
Feb 09 2016 06:33
Fair enough :)
Well, gotta run. I'll report back here.
Viktor Hedefalk
@hedefalk
Feb 09 2016 07:49

Just noticed I hadn't cleaned up properly after moving to linter. Here that is: hedefalk/ensime-atom@2d206e5.

@jeffwilde, sorry to remove that really nice code :'(

Richard Dallaway
@d6y
Feb 09 2016 08:21
@hedefalk new linter thing is looking good! I’ll get going on the release notes!
@hedefalk did I miss anything?
Do we like animated examples like that? (Two in a single page is too much for my head… but I can turn them into documentation on ensime.org at some point)
Viktor Hedefalk
@hedefalk
Feb 09 2016 09:17

@d6y Awesome!

By the way, the extra red thing in the gutter is removed in a later commit hedefalk/ensime-atom@2d206e5. That was our own, but linter just use that red circle.

Richard Dallaway
@d6y
Feb 09 2016 09:17
Ah ok - is what I’ve show accurate for 0.34.0 ? (assuming that has gone out)
Viktor Hedefalk
@hedefalk
Feb 09 2016 09:17
yes!
Richard Dallaway
@d6y
Feb 09 2016 09:18
okeydokey. Then I’m happy :-)
Viktor Hedefalk
@hedefalk
Feb 09 2016 09:18
But maybe I should do a point release to get rid of that extra gutter red. Or maybe not.
Richard Dallaway
@d6y
Feb 09 2016 09:18
Sure - knock yourself out. These release note things are easy to write.
Viktor Hedefalk
@hedefalk
Feb 09 2016 09:19
I think I'll try to fix the whole "contentsIn instead of content over the wire"-thing first before another release. I think that will make everything a lot better.
Richard Dallaway
@d6y
Feb 09 2016 09:19
I’ll be away for most of the day hacking away with Atom+Ensime on a project. I’ll report any weirdness I see.
Viktor Hedefalk
@hedefalk
Feb 09 2016 09:19
@d6y Awesome and thanks a lot for doing them!
Richard Dallaway
@d6y
Feb 09 2016 09:20
Yeah - sounds good!
Thomas Meijers
@ThomasMeijers
Feb 09 2016 10:45
Hi everyone. I love the work that is done on ensime-atom project, thanks :)! I was playing around a bit to implement the quick import feature, #127. Any suggestions/ideas on how to implement it? getImportSuggestions already exists and I was using autocomplete-plus to give it a view, however it feels a bit hacky...
Viktor Hedefalk
@hedefalk
Feb 09 2016 11:32

@ThomasMeijers Hi Thomas! Glad you like it!

What's missing for import suggestions is UI. As you saw, I have added the API call to get a list but then I just picked the first suggestion as a super-quick-hack when I was "doing real work". I agree that autocomplete-plus is not the right tool to create a view, other than maybe to look at for inspiration. I think what we want to do is create a general ui component for that kind of dialogs. "select something from a list that is shown at a point of the text editor/cursor". For instance, we also have the "show implicits" functionality that currently just displays a list without any actions on it. However, I'm thinking this could later be an "expand implicit" function when selecting one such item.

Anyway, I think we should try to create a coherent UI with components we can re-use for different use cases. Personally, I was planning to implement this using vuejs. (vuejs.org) which I have used half-arsed for a few use cases currently, like here: https://github.com/ensime/ensime-atom/blob/master/lib/views/public-symbol-search-vue.coffee

I want to remove code duplication and be dry about it though. Right now everything is a bit of a mess.

I don't want to specify to much though as this probably just makes it harder for you or anyone else to feel confident in contributing. I'm game for anything that makes it better than currently and when have time we can re-design if we want. So if you have any idea that works - go ahead!

Matthew de Detrich
@mdedetrich
Feb 09 2016 11:36
@hedefalk If you want, you can just ping me when you have address the imports issue
I always lurk around here anyways
Viktor Hedefalk
@hedefalk
Feb 09 2016 11:58
@mdedetrich Sure thing!
Thomas Meijers
@ThomasMeijers
Feb 09 2016 12:54
@hedefalk Sounds good, I will have a look at vuejs and try to come up with something reusable. Thank you
Viktor Hedefalk
@hedefalk
Feb 09 2016 13:56

I just noticed one thine: The package default keymapping of cmd-shift-T to ensime:search-public-symbol seems to clash with a new core binding to reopen last close pane giving really weird results. If you want to use that feature with that keymapping, paste

'atom-text-editor':
    'shift-cmd-T': 'ensime:search-public-symbol'

or similar in your private keymap file.

Viktor Hedefalk
@hedefalk
Feb 09 2016 14:16
I've removed all the contents-over-the-wire stuff now, but issues seems to remain. I also struck some performance issues in Atom. Feels like server is shooting messages like a gatling gun. Need to profile why atom is responding so badly to the load.
Matthew de Detrich
@mdedetrich
Feb 09 2016 14:18
Atom doesn’t have enough webscale, thats why
Viktor Hedefalk
@hedefalk
Feb 09 2016 14:18
:)
Matthew de Detrich
@mdedetrich
Feb 09 2016 14:19
Ah that reminds me, I am going to make a feature request
Viktor Hedefalk
@hedefalk
Feb 09 2016 14:21

I think the problem might be the linter. I struck some outofbounds-things with my ugly hack here:
https://github.com/ensime/ensime-atom/blob/v0.34.0/lib/features/typechecking.coffee#L15

I knew I would sooner or later. Problem is that we get range as beginning row, col + beg, end in absolute character offsets while the linter uses the Atom style of [[row, col], [row, col]] for a range. We can't actually translate that without reading the files which is kind of something we don't want to do I think.

@rorygraves @fommil Would it be possible to get range from server as [[row, col], [row, col]] too? Currently it's like: {"beg":7429,"line":207,"col":57,"end":7435} which simply doesn't cut it. To be able to use that we need to open up each and every file which kind of sucks performance-wise.
It was ok before since we had power over it and only calculated when a editor was open. But using the linter API we need to translate in another context.
Matthew de Detrich
@mdedetrich
Feb 09 2016 14:26
Just opened ensime/ensime-atom#157, not super urgent, but a good nice to have
Anyways im off to bed, catch you guys in the morning!
Jeff Wilde
@jeffwilde
Feb 09 2016 15:09
@hedefalk, nice work on integrating linter! Glad to see my code go the way of the bitbucket if it’s replaced with something prettier. This range translation prob sounds pretty hairy, though… always another problem, eh :|
Did I understand correctly that @mdedetrich is going to clean up the redundant gutter styling? I’d be happy to do that if I misread who was picking that up.
for future thinkings: it would be cool to look into if it’s possible to custom-style linter’s gutter markings, since I imagine a case where we’re want something more than just a coloured dot (and because i really like the look of our current styling)
Viktor Hedefalk
@hedefalk
Feb 09 2016 15:17
@jeffwilde I have made the cleanup myself and will PR soon. Would be pretty easy to add it to your own custom less though?
Rory Graves
@rorygraves
Feb 09 2016 15:19
It would definately be possible to calculate row/col on the server side rather than client side. Sublime has the same issue.
@mdedetrich Ensime-Sublime has a feature not in one of the other editors? I almost fell off my chair ;)
Viktor Hedefalk
@hedefalk
Feb 09 2016 15:22
@rorygraves Yeah, it kind of seems like it is done already for begin-pos, just not for end-pos. Quick fix? Will I break all the things if I just try to stuff it in there?
Jeff Wilde
@jeffwilde
Feb 09 2016 15:22
@hedefalk, ok cool. Just wanted to make sure it was on somebody’s plate after I heard it mentioned. Re. customization of linter’s display: Great if it’s that easy, I haven’t looked at what it provides, tbh. If somebody doesn’t get to it before me, I will take a stab at it at some point.
Viktor Hedefalk
@hedefalk
Feb 09 2016 15:23
@jeffwilde I think the styling we had could probably be used to hook into the linters DOM thingies the same way our own inline stuff was made.
But dunno. I'm pretty happy with just to red dot :)
Rory Graves
@rorygraves
Feb 09 2016 15:33
@hedefalk Its not going to be quite that simple (is anything). The Note class is created here https://github.com/ensime/ensime-server/blob/master/core/src/main/scala/org/ensime/util/Reporter.scala#L55 based on the underlying scalac pos, all of the methods you need appear to be private....
Adding fields to the result should have no impact (its json at the end of the day)
I suspect a bit of hacking around would provide a good way to handle it.
Viktor Hedefalk
@hedefalk
Feb 09 2016 15:42
Man, that nsc code makes me scared. It's so messy.
Jeff Wilde
@jeffwilde
Feb 09 2016 16:50
@hedefalk, I don’t mind the red dot so much as I was thinking we would have occasion to add other differentiators to them as the ide-like features grow.
Jeff Wilde
@jeffwilde
Feb 09 2016 17:26
e.g. let’s say we were to integrate scala clippy (https://github.com/softwaremill/scala-clippy) to augment the error messages, I’d like there to be some kind extra indication in the gutter icon that that error had extra explanatory info (god forbid an actual paperclip icon overliad onto the red dot O_o ).
Ghost
@ghost~540393fe163965c9bc2018ce
Feb 09 2016 17:28
@hedefalk can you not transform the json into the form you need or is info missing?
Jeff Wilde
@jeffwilde
Feb 09 2016 17:38
@fommil, i believe transforming it client-side is more expensive than we’d like.
Jeff Wilde
@jeffwilde
Feb 09 2016 17:47
(if that’s what you’re asking)
Viktor Hedefalk
@hedefalk
Feb 09 2016 18:48
@fommil What @jeffwilde said. Basically, we can't translate to row, cols from offset without reading file contents which might be a bit expensive if there's a lot.
But all that info is there on server, just behind crappy nsc.Position crap.
To be super clear, server gives: {"beg":7429,"line":207,"col":57,"end":7435} which is col, line and offset for start pos but only offset for end pos. We can't know if error spans multiple lines without opening up the actual file. When having 100+ warnings or so it kind of sums up to a bit of work on each update of messages from server.
Viktor Hedefalk
@hedefalk
Feb 09 2016 18:54
Which seems quite obvious to be double work since server already have this info.
But it seems a bit obscured. scala.reflect.internal.util.Position seems like a mess to me.
Ghost
@ghost~540393fe163965c9bc2018ce
Feb 09 2016 19:05
Ah, i see. Sorry, Scala compiler only gives offset. Never been a problem for Emacs because it can work with offsets.
We could calculate it server side but that might be trickier than you'd expect
Rory Graves
@rorygraves
Feb 09 2016 19:09
Two thoughts - first, this should be fairly easy server side, the RangePosition already has the methods to get the start line, col. I think it would be a bit of fiddling to get the end pos to line as well. Second, given the starting line and offset and pos, there is a shortcut to calculating the second one (you only need to do it from the start position, not the start of the document. (generally the ranges are quite small)
Ghost
@ghost~540393fe163965c9bc2018ce
Feb 09 2016 19:11
Oh that'd be nice. If we could include the line start in the response, that'd work well.
Rory Graves
@rorygraves
Feb 09 2016 19:13
we include the line and col of the start pos, but not the end pos.
Viktor Hedefalk
@hedefalk
Feb 09 2016 19:21

Yes, it's

```

  def line: Int           = if (hasSource) source.offsetToLine(point) + 1 else 0
  def column: Int         = if (hasSource) calculateColumn() else 0
both defined in terms of point
Don't get it though, RangePosition has three points, start, point and end, but I would guess point == start.
So, can't we just create a temp copy point from end and use those methods?
Rory Graves
@rorygraves
Feb 09 2016 19:25
Something like that yes
had not I think you are right that start==pos
Viktor Hedefalk
@hedefalk
Feb 09 2016 19:28
val end = pos.withShift(pos.end-pos.start)
end.line, end.col
Rory Graves
@rorygraves
Feb 09 2016 19:28
blimey, that was easy
Viktor Hedefalk
@hedefalk
Feb 09 2016 19:28
maybe, i'm coding in markup :)
markdown.
Rory Graves
@rorygraves
Feb 09 2016 19:30
Yeah, but you do it with conviction so everybody things you know whats going on ;)
Viktor Hedefalk
@hedefalk
Feb 09 2016 19:31
:)
Richard Dallaway
@d6y
Feb 09 2016 21:17
@hedefalk yeah, I ran into the clash with Reopen Last Item today. Seemed to alternate between bringing up symbol search or the last item. I guess remove the binding? And I’ll add a note on the feature page on how to create a binding.
When I write a feature page, that is.
Other than that - I like the new linter thing!
Viktor Hedefalk
@hedefalk
Feb 09 2016 21:56
There might be issues with current release with regards to performance of linter. I have a fix soon to be released.
Ghost
@ghost~540393fe163965c9bc2018ce
Feb 09 2016 21:57
sorry I missed the conclusion about the performance stuff, does the server need a patch and a protocol change?
Rory Graves
@rorygraves
Feb 09 2016 21:58
Depending on the code in atom there is probably some hacks you could do, but it would be easy to add the end line/col to the protocol and return them.
Viktor Hedefalk
@hedefalk
Feb 09 2016 22:05
@fommil That was something entirely different in UI land. I left that col, line thing for now. This atom-community/linter#1075
Basically I have to do: setMessages(JSON.parse(JSON.stringify(messages)))to not make linter blow up my CPU :)
Ghost
@ghost~540393fe163965c9bc2018ce
Feb 09 2016 22:06
@hedefalk can you please raise a ticket for the col/line thing? Flag it Low Hanging Fruit if you can (otherwise me / @rorygraves can) and maybe somebody can work on it at the hack day on Saturday... Atom is becoming very popular
we should add it to 1.0 milestone though if its important for atom
Rory Graves
@rorygraves
Feb 09 2016 22:07
@hedefalk Is our JSON badly formed?
Viktor Hedefalk
@hedefalk
Feb 09 2016 22:09
v0.35.0 OUT!
@rorygraves No, it's a weird thing with atom linter api that i can't use setMessages with messages that are already there as in object equality, then some caching algorithm makes everything go bananas. So I have to create new objects all the time on each update of the messages. Since ensime-server gives them incrementally, I need to store them somewhere but can't reuse as is without creating new objects. Very strange and weird I would say, but it seems fine performance-wise with JSON.parse(JSON.stringify(messages))
it's fixed in 0.35.0. GET IT OR BE SQUARE!
Rory Graves
@rorygraves
Feb 09 2016 22:12
ouch, thats nasty
Ghost
@ghost~540393fe163965c9bc2018ce
Feb 09 2016 22:14
@hedefalk tweet and we'll retweet
Viktor Hedefalk
@hedefalk
Feb 09 2016 22:14
@fommil I'll raise ticket tomorrow! Remind me if I forget :)
Gotta go to bed.
Ghost
@ghost~540393fe163965c9bc2018ce
Feb 09 2016 22:17
pffh, I don't even remember to go to bed, don't know what you're asking me for :-P
Richard Dallaway
@d6y
Feb 09 2016 22:52
Please go edit them if I’ve got anything wrong, or missing out anything. I don’t have to say that, right? People will just go do it.
Richard Dallaway
@d6y
Feb 09 2016 23:03
…and nice detective work, re json, getting 0.35.0 out. :dancers: :dancers: :dancers:
Matthew de Detrich
@mdedetrich
Feb 09 2016 23:09
@jeffwilde I don’t think I said that I was cgoing to clean up the guttter styling
Jeff Wilde
@jeffwilde
Feb 09 2016 23:16
Yeah, I was not certain who said what in my skim of the discussions, I just knew removing the redundant styling had come up, and I wanted to make sure somebody was on it.
hedefalk did ultimately claim it (maybe it’s even in this 0.35.0 release)
Matthew de Detrich
@mdedetrich
Feb 09 2016 23:18
What did 0.35.0 fix?