Bookmark icon is covered by Gutter #76

Closed
jiyinyiyong opened this Issue Mar 21, 2013 · 44 comments

Comments

Projects
None yet

Might be not really a bug though,..
If I try adding bookmark in the same line where Gutter highlights, there would be no response of bookmarks. I suppose the space at a line start can only hold one icon. While there are two, they just conflct.
Then there's a problem: I can't tell from the line wether there's a bookmark, that's annoying.

lorin commented Jul 5, 2013

I hit this as well. Would love it if bookmarks would appear instead of git gutter marks.

Owner

jisaacks commented Jul 5, 2013

Thanks for brining this up, I never noticed because I have never used bookmarks before. I am not sure if there is a way to determine if a specific line already has an associated region (icon), or a way to get the key of such region.

If there is, then I could add a setting to GitGutter that allows you to specify which regions not to overwrite. (e.g. bookmark, bracket-highlights, etc.)

I took a glance at the API and all I see is the View.get_regions(key) method. I will have to experiment to see if I can use the keys set in a setting to determine if a specific line has a region for that key, and if so not to mark it.

Owner

jisaacks commented Jul 5, 2013

I just notcied the original issue was reported months ago. I guess it just wasn't on my radar until it just got the bump by @lorin

I am getting hit by this and the Linter issue #113 also.
Can the gutter be made wider?

Contributor

DisposaBoy commented Sep 28, 2013

I'm pretty sure this is a Sublime Text issue/limitation. i.e there can be only 1 icon there at a time.

That sounds rather weird, but for some reason prefixing all icon files with, say, "icon_" (and changing icon_path method accordingly) fixes the original issue. =~

P.S.: Sublime Text 3, build 3054

P.P.S.: Sorry, it doesn't seem to be a robust fix, only some random types of mark gets covered with a bookmark. Still can't understand which and when exactly.

Owner

jisaacks commented Nov 7, 2013

@abusalimov I have no idea what you are talking about

on a side note: The possible solution I referred to a while ago, I have tested and confirmed it would work. However you would have to specify what regions were protected for GitGutter to not override them. Then anytime GitGutter is about to add an icon, it would have to grab all the regions for all the protected regions (each protected region could have many regions of that type on the screen) and make sure that non of them occupy the space that GitGutter wants to put an icon. I am afraid of this slowing GitGutter down, so I am not sure if this is a feature I want to add. It may only be useful in non blocking mode, which is only available in ST3.

Also the OP states:

I can't tell from the line wether there's a bookmark, that's annoying.

If you told GitGutter to not override then specific regions you would have the reversed problem:

I can't tell from the line wether there's a change, that's annoying.

So again, not sure if this feature is worth it.

@jisaacks I don't think this would slow down GitGutter. All you need is to create a set of protected positions (beginnings of bookmarked lines) and check whether a position to be added is not in the set. Not a big deal for few regions types ("bookmarks", "mark", ...). Proof-of-concept: https://gist.github.com/abusalimov/7363673

I don't insist that this is a show-stopper thing. But personally I have both VcsGutter and GitGutter disabled only because they break my daily workflow making bookmarks unusable. Again, I understand that many developers dont't bother when bookmarks get hidden or don't use bookmarks at all.

P.S. It's a pity that Sublime can't combine two png images using their alpha channels. That would completely solve the issue without tricky hacks.

lorin commented Nov 8, 2013

I can't tell from the line wether there's a bookmark, that's annoying.

If you told GitGutter to not override then specific regions you would have the reversed problem.

For what it's worth, I'd prefer bookmarks to take precedence over git gutter markings, since it's easier for me to toggle a bookmark off to check if there's a git gutter marking (the bookmark keyboard shortcut is in my muscle memory) than vice versa.

Owner

jisaacks commented Nov 8, 2013

@abusalimov I understand the logic required to make this work, I have a working branch locally with the functionality. And in my code as in your gist (in the lines_of_regions method) it requires a nested for loop.

Lets say the user has specified 30 protected regions (they could easily if using SublimeLinter and BracketHighlighter) and lets says each protected region type has 5 actual regions by that name on the view.

That would be 150 calls to lines.update(line.begin() for line in self.view.lines(region)) just to check if it is OK to place an icon. And it would need to do this for every keystroke if using live mode.

I am not saying no to this feature, but these are the things that affect the decision. I may just add the feature with a warning that I recommend only using this feature in non blocking mode (which is ST3 only.)


@lorin I have also considered a toggle for git gutter to clear the icons, that would allow you to toggle back and forth between viewing git_gutter/bookmarks/whatever

@jisaacks well, you are talking about the most general solution where the user could specify a list of protected regions. I'm sure that with resonable defaults provided (like with only bookmarks and marks taking precedence) and a warning about adding more region types this feature would be perfectly fine. After all, I'm pretty sure that counting even 1500 markers would be still faster than getting a fresh diff each time (which forks a subprocess for that). Anyway, the final decision is up to you, of course.

Owner

jisaacks commented Nov 9, 2013

@abusalimov I added a branch called protected-regions it defaults to no protected regions so you would need to specify any you want to protect in your user settings. Please check it out and let me know your thoughts.

@jisaacks thank you.

I can see that in your solution you read a setting and collect protected regions for every single line of the resulting diff which in turn can be rather big. I don't think that having lots of protected regions is real (see my previous comments), but I believe that there can be a huge diff with thousands lines. And that can really hurt the performance.

In the solution that I've suggested above you only need to prepare a set of protected regions once. After that a simple set lookup (which is really fast in Python) is performed for each diff line to be marked.

Owner

jisaacks commented Nov 10, 2013

@abusalimov

for every single line of the resulting diff

Not exactly, just for every line that has changed and GitGutter wants to place an icon on. But either way, you are still correct, better to only calculate it once.

Would you like to make a pull request with your version?

@jisaacks well, I'm new to github, and I'm afraid I'll get some free time to learn it only on the next weekend. So if this change is not too urgent, I could do it within a week.

I also noticed that in my solution (I guess, in your too) there is a little bug: when removing a bookmark on a changed line this line would have no mark at all (because changed mark was not added because of the bookmark). May be you know, is there any hook to intercept changes on certain regions?

Owner

jisaacks commented Nov 10, 2013

@abusalimov well in the case of boomarks you can listen for text commands via the on_text_command sublime text event listener and check the command name to see if it contains the string bookmark. That doesn't solve the general problem though.

It would be nice if in the method where we are checking for protected regions, if a protected region is found, place an event listener on the region for when it gets cleared. I don't think that is possible.

I'll keep thinking about it and see if I can come up with any clever ideas.

@jisaacks, I've just forked the repo. Should I commit into the protected-regions branch (maybe overriding changes you already made) or into the master?

Owner

jisaacks commented Nov 18, 2013

Lets keep this feature in the protected-regions branch for now.

+1 here. Is the current status still in protected-regisions branch?

Owner

jisaacks commented Nov 6, 2015

I merged the protected-regions branch into master. However, I am trying to get people to test it out and report back to me (using GitGutter-EDGE) before I cut a release. Unfortunately, I haven't had a report of a single user installing and using protected-regions.

I've tested your fix in the context of SublimeLinter and it seems it's working. I wish I had more time to do code review, the code requires optimization in my opinion.

After installing GitGutter-Edge add following to user settings

"protected_regions": ["sublimelinter-error-gutter-marks", "sublimelinter-warning-gutter-marks"]

Now someone should extend the list of regions by bookmarks etc and update readme file.

Owner

jisaacks commented Nov 11, 2015

Thanks @develucas, what optimizations were you thinking?

g105b commented Apr 24, 2016

Hi all. I have been following this thread for a while as I use Sublime Linter and XDebug (which places debug breakpoints in the gutter).

Tried multiple things over the past weeks to avoid having GitGutter eat up the gutter icons, and today I tried removing Githutter completely and installing manually.

After installing manually (cloning the repo into the Packages directory), Gitgutter no longer takes preference over the other gutter icons!

So whatever you've changed works. But please note that GitGutter-Edge still has the issue for me (Ubuntu 15.10 64bit).

g105b commented Jun 1, 2016

I'm back. GitGutter has started drawing gutter icons over the top of the bookmarks and sublime linter / xdebug breakpoint indicators.

This issue is still open for me. Here's the contents of my GitGutter.sublime-settings (which makes no difference):

{
    "protected_regions": [
        "sublimelinter-warning-marks",
        "sublimelinter-error-marks",
        "sublimelinter-warning-gutter-marks",
        "sublimelinter-error-gutter-marks",
        "lint-underline-illegal",
        "lint-underline-violation",
        "lint-underline-warning",
        "lint-outlines-illegal",
        "lint-outlines-violation",
        "lint-outlines-warning",
        "lint-annotations",
    ]
}

Likewise, the protected_regions setting is not working for me. SublimeLinter gutter icons are being overridden with GitGutter gutter icons, despite my having specified:

{
  "protected_regions": [
    "sublimelinter-warning-gutter-marks",
    "sublimelinter-error-gutter-marks"
  ]
}

ocrow commented Oct 20, 2016

GitGutter.sublime-settings has the following, but bookmark icons are still overridden by GitGutter.
Is this feature working?

  "protected_regions": [
    "sublimelinter-warning-gutter-marks",
    "sublimelinter-error-gutter-marks",
    "bookmarks"
  ]
Collaborator

r-stein commented Oct 20, 2016

@ocrow Do you use the normal bookmarks or have a bookmark package? Normal bookmarks should work and works for me.

ocrow commented Oct 20, 2016

I'm using normal bookmarks. SublimeText 3126; GitGutter v1.3.0.

Collaborator

r-stein commented Oct 21, 2016

@ocrow How does the line look like and where do you add a bookmark? The region detection should work for the whole line with #304, but might still have some flaws.

ocrow commented Oct 21, 2016

I see a small greenish square in the first column to the left of the line number of any line that has been modified since the last commit. If I add a bookmark on a line that has not been edited, it appears as a large white right-pointing triangle arrow.

If I add a bookmark on any line that has been edited, the white bookmark arrow does not appear, which is the problem. It does not make any difference whether the bookmark is at the beginning of the line, the end of the line, or in the middle of the line. Similarly the bookmark may be for a single position or a selection. In each case the bookmark icon does not appear iff the start of the bookmark is within a line that was already marked by GitGutter as edited.

If I add a bookmark before editing the line, the bookmark arrow stays and the 'modified' square does not appear. I think that is expected behavior.

ocrow commented Oct 26, 2016

I removed my GitGutter user configuration, uninstalled & reinstalled GitGutter, restarted SublimeText, and all of that seems to have done the trick. I now can see bookmarks on top of GitGutter modified icons.

Collaborator

r-stein commented Oct 26, 2016

@ocrow Good that it works now =)

Also this whole issue should be fixed and I close it.

r-stein closed this Oct 26, 2016

g105b commented Oct 27, 2016

Activity on this thread made me try Gitgutter again after a long time. Worked really nicely at first, but it seems like I have to remove and reinstall Gitgutter multiple times per week to maintain consistent behaviour with bookmarks.

Collaborator

r-stein commented Oct 27, 2016

@g105b reinstalling GitGutter does not change the code, so it's weird that this would fix something. If you can provide reproducable examples where it fails, I would be glad to fix those.

g105b commented Oct 27, 2016

Will do - I'll keep my eye on it. Is there any logging I can turn on so I can provide these as soon as I notice gitgutter going over the top of bookmarks?

Collaborator

r-stein commented Oct 27, 2016

There is no logging command and this is more about overlapping regions. You can paste this into the ST console and it should output all relevant informations about regions and lines:

region_names = ["bookmarks"] + ["git_gutter_" + r for r in ['deleted_top', 'deleted_bottom', 'deleted_dual', 'inserted', 'changed', 'untracked', 'ignored']]; [("lines", view.lines(sublime.Region(0, view.size())))] + [(r, view.get_regions(r)) for r in region_names]

In addition it would be nice if you could provide a small description in which order you have added the boomarks and the gitgutter informations.

g105b commented Oct 27, 2016

Roger that. Will reply back when the problem happens again. Still working fine today.

redochka commented Dec 5, 2016

Problem still there. Please check

jhakonen commented Apr 5, 2017 edited

I seem to have the same problem. Seems to be working partially in my case.

This works as expected:

  1. Add a bookmark to a line, bookmark icon (white right pointing arrow) appears on the line
  2. Modify the same line, bookmark icon remains, as expected

However, this does not:

  1. Modify a line, GitGutter's changed icon (yellow square) appears
  2. Add a bookmark on the same line, changed icon remains, should have changed to bookmark icon, but did not.
  3. WORKAROUND: Modify another line, changed icon appears. At same time the previous line's icon updates from GitGutter's changed icon to bookmark icon.

Using ST3 3126, GitGutter 1.5.1.

Collaborator

deathaxe commented Apr 5, 2017

GitGutter can't track other plugins adding or removing gutter icons and thus can't react on it. It only checks for existing gutter icons (protected_regions), if it is about to update its own gutter icons.

It is an intended behavior of GitGutter so far to update its gutter icons only, if either the view's content or the file from database has changed. The whole evaluation is aborted, if none of the files has changed to not unnecessarily run git and/or write file content repetitively to disk.

Anyway, adding/removing bookmarks doesn't trigger any event, which would cause icons to update. A view must be (loaded, cloned), activated or modified to trigger GitGutter to evaluate changes.

Collaborator

r-stein commented Apr 5, 2017

It is very weird that it overwrites "added" and "deleted" icons, but not "changed" icons. We should have a look into the reason for that and try to adapt our way of adding the "changed" icons accordingly. It may have to do something with the name or the icon path.

Collaborator

deathaxe commented Apr 5, 2017 edited

The reason is, that the bookmark icon is simply added to the line, where a GitGutter icon already exists. So they fight against each other - maybe by name - and GitGutter seems to win sometimes. All GitGutter region names start with "git_gutter_" so I don't have an idea, why bookmarks should have a "priority" in between of "added/deleted" and "changed" regions. The only possible reason would be ST sorting by 'scope' name but not 'region_name'. The scopes are named like 'markup.added.git_gutter'. I'll have a look on it, but won't change scope names because of backward compatibility.

The only way was to check when one of the protected regions has changed and update gutter icons to avoid fighting. :-/ Some kind of messy.

P.S.: For me bookmarks overwrite all gutter icons (added/deleted/changed).

Collaborator

r-stein commented Apr 5, 2017

You could also name the scope aaaaa, markup.inserted.git_gutter to keep backward compat. One could also create an EventListener to refresh the git gutter regions after some commands are executed.

import sublime_plugin


class BookmarkGitgutterListener(sublime_plugin.EventListener):
    def on_post_text_command(self, view, command_name, args):
        if command_name == "toggle_bookmark":
            view.window().run_command("git_gutter") # not sure which arguments to pass to refresh the icons
Collaborator

deathaxe commented Apr 5, 2017

This might address the problem with bookmarks, but we would need something to work for all protected_regions - just to keep consistent. As a user can define its own list of regions, this might be a quite tricky task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment