-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Inline marks #1297
Inline marks #1297
Conversation
Thanks, I have only done a very superficial perusal of the commits, but I see that some commits contain leading spaces instead of tabs, and also some commits should probably be squashed into a previous one, e.g. when a commit starts with fix, simplify, or rename “something done in previous commit”, then it’s a candidate for a fixup instead, since I want to only review the functional change from current master to your final version (of that functional change), not each step that took you to the final version. Makes sense? |
Yeah, I noticed that, I'll fix it.
Yes, absolutely. Do you still want multiple commits or should I squash to just one? I noticed that there was a lot of changes in the |
I think the PR can be split into these 5 changes (the last four are already standalone commits):
Btw: Per git commit guidelines, the commit message summary should not be terminated with a period. Something we should probably make the git commit message grammar catch (@infininight).
What I request is that xibs are first saved without making any changes and committed, then the change is made — this makes it easier to review the actual xib change and possibly revert/merge it. See https://github.com/textmate/textmate/blob/master/CONTRIBUTING.md#changing-a-xib-file |
Thanks, I'll fix all this. |
73b6cf2
to
f504f14
Compare
I think I have addressed all comments:
I updated the PR description with some more info as well. |
@sorbits do you plan to merge this in the near future? This feature looks really promising. I'll gladly work on a JSHint bundle that supports this. |
I certainly intend to incorporate this PR, but I can’t say when I will On 4 Jun 2015, at 1:08, Igor wrote:
|
Awesome, please let me know if there's anything I can do to speed up the process. |
I finally got around to take a closer look at the commits. There seems to be 3 functions added:
I think this should be properly broken out into distinct commits. For the first feature, I don’t think this should be tied to inline marks. The current implementation of this feature lack to handle the case where no theme color is set. For example I now have all lines with marks drawn with a white background, despite using the (default) dark theme. It could probably use the default gutter view highlight line color. Also the property should probably be named something with highlight rather than just line background. Furthermore, for C++ instance variables I use Some other comments:
Some concrete issues: In
In
In
Indent is also increased after |
Thanks for all the comments.
It's been a while since I worked with this code now but I don't understand the difference between number one and three, "Ability to highlight lines with marks" and "Rendering of the inline marks". I see that you're referencing commits which don't exist: 95fbf90 and d38567c. Are you not looking at the most up to date code? But I still see that there are commits which have the problems you're mentioning, just with different hashes. I'll take a look at the other issues. Thanks. |
Sorry about the references. I had rebased your work ontop of (my) master As for 1 and 3. Number one adds the ability to draw lines with marks This is irrespective of whether or not the mark has any associated text, Number 3 would be drawing the text inside the editor window On 5 Aug 2015, at 14:56, jacob-carlborg wrote:
|
Ok, I see. But where would the background color for the text (the darker red in the screenshot) be handled? It's dependent on the width of the text. |
Without having verified what the view port size property in the layout contains, I would guess it does not include the margin. I know I had some issue in the beginning with the red highlight not being drawn all the way out to the gutter view. |
The view port size is the “visible rect” as set here: https://github.com/textmate/textmate/blob/master/Frameworks/OakTextView/src/OakTextView.mm#L802 If there is horizontal scrolling then it excludes the space outside the visible area, however, the layout’s own width would provide the actual width. |
lineBackground is intended to be drawn on the whole line.
It's kind of hard to tell if the images are correct, because of the colors. If lineBackground is drawn with a white color and "background" is drawn with the same color as the text editor background color then both look correct. Could you please try add a new theme setting for the scope "mark.inline.error" with some values for "foreground", "background" and "lineBackground" for the theme you're using. |
If lineBackground is supposed to be drawn for the whole line (regardless As for the white background, this is the out-of-box experience. I think On 5 Aug 2015, at 17:42, jacob-carlborg wrote:
|
I don't know, you can call it whatever you want 😃 . My goal was to do something that looked how the error messages in Xcode look like. What would be the point to color a line without the inline text?
As I wrote when I created the pull request, that we (I) should extend the default themes with a couple of default scopes, |
It’s unfesable having to update all themes out there (many are not The gutter view colors are derived from the main theme colors, something As for the line background coloring not making sense without the text: Try switch to the Twilight theme and then do a folder search for On 5 Aug 2015, at 19:25, jacob-carlborg wrote:
|
It appears that the line above bookmarks are also drawn with the Try set a bookmark and then force a redraw of the line above (e.g. select/deselect it). |
Ok, great 😃
That's not the intention. Hmm, I guess I haven't noticed since I'm using white as the editor background color. I think I thought that the theme fell back to the editor background color if So either the code needs to somehow know which lines to color or color all lines (which have marks) and fall back to the editor background color if the scope isn't present in the theme. |
Is there a technical reason for extending the API and allow “appending” marks? I.e. wouldn’t a client not always start by clearing all marks in the category (e.g. linter warnings) and then set new marks? So in the rare case there are multiple marks for the same position, the client could just provide them as newline delimited, or if we want to support newlines in marks, they could use the record separator character: 0x1E. |
No, the only reason is to allow multiple marks per line. If this is not supported the client would need to bundle the marks per line. I'm not sure what you prefer. |
BTW, have you considered using clang-format and to have a style guide? |
Last I tried clang-format I was unable to install it. Do you know of an easy way to install it? As for style guide, I’m almost certain I have one somewhere, but I On 7 Aug 2015, at 10:05, jacob-carlborg wrote:
|
I’d prefer to keep the existing API as it is. If the client is likely to produce multiple marks for the same position If it turns out clients often need to handle multiple records for the On 6 Aug 2015, at 21:19, jacob-carlborg wrote:
|
Homebrew includes a formula for clang-format. By the way, thank you Jacob and Allan for your work. This certainly looks like a very interesting feature. |
I wouldn't consider that a style guide. Following the style of the existing code is something I always try to do, but in this case I missed a couple of things. For example I think it's quite unusual to use snake case for instance variables but camel case local variables and parameters. |
This component is intended to be used to color the background of a whole line. When "highlight" is used, "background" is intended to only color the background where there's text.
c69dbd4
to
780c1ed
Compare
To set the color of the highlight: 1. Add a new setting to the theme with the scope "mark.inline.<mark_type>", where <mark_type> is the mark type to be highlighted. 2. Add the theme component, "highlight", to set the highlight color. The following setting will highlight marks with the mark type "error" with a light red color. The mark type is used when setting a mark with the "mate" command: "mate -s error:foo". { name = 'Mark inline: error'; scope = 'mark.inline.error'; settings = { highlight = '#FFD3c5'; }; }
By default the inline marks are not rendered. A new preference, "Show inline marks" (a checkbox), has been added to the Files preference pane. When enabled, inline marks will be rendered.
780c1ed
to
1fdd408
Compare
@sorbits I think I have fixed everything you asked for, see the list at the top. Except for two things, see the last two comments #1297 (comment) and #1297 (comment). |
Thanks for the changes. As for the line color change for bookmarks, with the default highlight color now being the same as the background color, there is no issue. However, if you set a highlight color then every line with a gutter mark (bookmark, SCM added/changed indicator, folder search result, etc.) will be rendered with a changed background color. I don’t understand this highlight setting: We want to draw text associated with a mark inside the text area, we may want to give this text a different color or background (framed box of sorts). But why do we want to change the background color for regular text? |
I btw looked into That said, running |
I don't see that issue. How have you modified the theme?
I just wanted to highlight the whole row. Used for compile errors, indicating there's a compile error on this line. I've modeled this whole feature like how Xcode shows errors. |
I'm not telling you how you should write your code but I would prefer a tool that formats all code consistently automatically, even though it doesn't format it exactly like I would like.
That's unfortunate :(. One of the reasons I never liked alignment of code. |
I don’t understand why you are not seeing highlighted lines for all line with a gutter mark, because the code does draw it for all lines, regardless of whether or not there is any text associated with the mark. However, this code https://github.com/jacob-carlborg/textmate/blob/45f5ca49b4110f658c8d770f2abcd417a82580f8/Frameworks/theme/src/theme.cc#L285-L289 copies the background color into every style that has not set a highlight color, so if you are not in root scope, and there is a style setting for the scope you are in, then you would get the regular background color. Maybe that is the issue? As for code style; it would indeed be great to have a tool that dictates the style (like go), however, not making a distinction between prototypes and call sites would make searching (and replacing) much harder (which I use a lot), and not aligning code harms readability and also makes it much harder to apply batch actions to the (aligned) code / data structures. |
For the patch itself, things generally look fine. I would change the code that initializes every style’s highlight color, as copying the background into every style removes the blending ability, for example, instead of the loop, I would just do this:
Although I am still not entirely sure about this “highlight” color. The gutter mark is already supposed to call attention to the line. There is a usability issue with the code in that long errors (which is not uncommon from clang) will eclipse the entire original line. There is also a refresh issue when there are multiple warnings/errors. We could perhaps ignore the refresh issue, because it’s not easily solvable with the current layout system. I think it would be better to add a view (NSView) to the text view, that is the “error box”, then it could also have a close button etc., rather than have the (text) layout handle these “boxes”. But using an NSView could be a “version 2” of this feature; this is also one of the reasons I was so obsessed about minimizing the (internal) API changes, because I would consider this implementation somewhat interimistic. |
So the code automatically falls back to the color of the root scope if it's missing in the current cope?
Yeah, that is a problem. Although the code tries to be a bit smart in this regard. If the mark text will cover the code on the current line it will try to draw it on the next line. If it will cover the code on the next line it will just draw and cover the current line. This is the naive implementation. It's also one of the reasons why it's disabled by default. A better approach would be to also wrap the code or the text if the mark text would cover the code. I never figured out how to do that.
When does that occur? When scrolling, changing window size?
Would that new view kind of float above the view? |
Correct.
If the text is drawn below the line it’s attached to, it would happen every time we do not redraw both lines at the same time. For example editing or selecting at the line below the mark (but where the text is) would not draw the mark’s text. Also scrolling (slow) should fail to draw the mark’s text when the line below is moved into the view port.
It should be possible to add an NSView to the OakTextView, as a subview, this would then scroll with the text (which is scrolled around in its containing NSClipView/NSScrollView). The main issue is managing these views, i.e. update their position when lines are inserted/deleted. |
I noticed that now.
Do you think that would be better? I noticed that there's also a problem when scrolling, the mark doesn't follow the edge of the view. |
Hard to say without having tried it, but right now the layout code is very geared to solving the problem of minimal layout (of text metrics), minimal redraw, and delta updates. Placing “boxes” in various locations seems somewhat orthogonal to what the layout code is about, but the NSView system is very well suited for this task, plus one can easily add interactivity to the boxes using the NSView system (e.g. a rollover close button).
I believe the issue is related to this code:
The |
I'll give the NSView approach a try and see what how it works. |
Merged? This was a surprise. |
Ups… I pushed the wrong branch, thanks for catching that. I’m not opposed to merging the current stuff because the impact is On 28 Aug 2015, at 8:16, jacob-carlborg wrote:
|
I don’t see a way for me to re-open this PR after it was auto-closed :( |
@sorbits I've been thinking about the NSView approach. There's a problem with that approach. The background color of the inline mark needs to be drawn before drawing the text in the OakTextView. But if the inline marks are subviews of OakTextView the inline marks will be drawn after the OakTextView, i.e. on top of the text. What about drawing the background color with the current approach and draw the text using an NSView? |
To avoid code duplication I think |
Currently it's actually the layout framework that draws both the background and the text. I can think of a couple of approaches:
The block approach (the first one) seems to be the simplest approach. Unless you have some better ideas. |
I agree that the block seems like the simplest approach. I don’t know how much progress you have made but just a word of warning, I’ll likely revamp the layout stuff shortly. If you look at the current version of |
I've not worked on this at all since the my last comment. I'll wait until you've revamped the layout code. |
This pull request add supports for inline marks. The attached image shows an example of how it will look like. The implementation uses the existing marks API, meaning calling
mate --set-mark
will show up as an inline mark. Since this is a bit intrusive change it's disabled by default and there's a new preference available to enable it.Drawing of the inline mark tries to be a bit cleaver, it will draw the message on the next line if it will hide the code on the current line. If it hides the code on the next line as well, it will fall back to draw on the current line.
A new theme component, "lineBackground". It's used to color the background of a whole line (the lighter red in the attached image). The existing "background" is used to color the background only where there is text (the darker red in the attached image).
We most likely want to extend the themes with a couple of default marks, like "error", "warning" and so on.
buffer/src/marks.cc
++row
instead ofstd::next
CGContextSaveGState(context)