Skip to content

Conversation

@twolfson
Copy link
Contributor

@twolfson twolfson commented Oct 13, 2016

As discussed in #64, adding left/right positioning support solves cursor jump issues. In this PR:

  • Added toggle support via inline_preview_position
  • Added clarified variables (we can remove these if desired but they helped me understand initial intentions of code)
  • Fixes Prevent reflow while editing #64

Notes:

I haven't squashed this branch as GitHub now supports squashing commits. If you would like me to still squash it (e.g. to merge outside of GitHub), I'm fine with doing that.

@facelessuser
Copy link
Owner

I will review this when I get a chance, but I am curious if you verified that switching back and forth introduces no issues? Does it all clean up proper?

@twolfson
Copy link
Contributor Author

Yea, when I update the settings, all previews are wiped and don't update until I focus back on the view

@facelessuser
Copy link
Owner

That's good, I couldn't remember if I reloaded all the previews when specific setting values change or all values. If this is all that is needed, I probably won't have an issue with this. I am glad I went the route of any value in the settings file changes. Can you update the documentation to talk about the new setting, that would be good too.

@twolfson
Copy link
Contributor Author

Added new documentation

color_helper.py Outdated
sublime.status_message('File color indexer started...')

def preview_is_on_left():
"""Return boolean for positioning preview on left/right"""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to resolve the lint error here by adding a period to the comment's end. Any errors showing in lint that weren't introduced by you can be ignored. Sometimes flake8 updates and introduces previously missed errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has already been resolved in 2c99c81 . Travis CI caught it and I patched it =/

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, I actually made the comment before you fixed it but finished the review yesterday.

end_scope = view.scope_name(src_end - 1)
rgba = RGBA(mdpopups.scope2style(view, scope)['background'])
rgba.invert()
color = '<a href="%d">%s</a>' % (
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't use pt anymore for href, you need to use src_start. Line #1191.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nice catch

@twolfson
Copy link
Contributor Author

Alright, href has been resolved and looks good after manual testing (as commented, right sided panels wouldn't load):

selection_244

selection_245

@facelessuser
Copy link
Owner

I think that's everything. Thanks for looking into this.

@facelessuser facelessuser merged commit cc754a5 into facelessuser:master Oct 31, 2016
@twolfson
Copy link
Contributor Author

Now that is landed, when should we anticipate a corresponding git tag/GitHub release?

@facelessuser
Copy link
Owner

Don't know, depends if I feel I have time to look into the duplication bug or not. If I do, maybe after that, if not, then before that. I've been pretty busy recently. I would say no not this week though, but maybe next.

@facelessuser
Copy link
Owner

Released

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants