Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Remove basePadding from indentwrap hack #12110

Merged
merged 1 commit into from
Aug 20, 2016

Conversation

petetnt
Copy link
Collaborator

@petetnt petetnt commented Jan 20, 2016

As noted by @redmunds on Slack there's an slightly ankward gap between the gutters and the text

image

This is due to the indent-wrap hack in #11909: the texts are pushed 4 pixels back by the hack. This PR compensates for that by pulling the editor back -4 pixels:

image

@abose
Copy link
Contributor

abose commented Jan 21, 2016

tagging @swmitra

@redmunds
Copy link
Contributor

Thanks for this fix. This is better, but I now see a gap between gutter and Active Line highlighting:

brackets-1 6-12110

@wyattbiker
Copy link

Same issue. Here is my original question and also temporary fix for now.

https://groups.google.com/forum/?hl=en#!topic/brackets-dev/6ri52cF2FB8

On Thu, Jan 21, 2016 at 10:24 AM, Randy Edmunds [email protected]
wrote:

Thanks for this fix. This is better, but I now see a gap between gutter
and Active Line highlighting:

[image: brackets-1 6-12110]
https://cloud.githubusercontent.com/assets/1197144/12484589/de1d9c7e-c00f-11e5-9a8d-e69a57b60a49.png


Reply to this email directly or view it on GitHub
#12110 (comment).

@petetnt
Copy link
Collaborator Author

petetnt commented Jan 21, 2016

@redmunds Which theme is that? Looks like the regular theme, but grayer? Cannot replicate on my machine.

However, I pushed in alternative (but similar) solution in b461a1a if you want to give it a try.

@lkcampbell
Copy link
Contributor

@petetnt has anyone figured out when this gap appeared and why is started happening?

I only noticed it fairly recently. If we understood what introduced the problem we could address it where it was introduced instead of hacking the style sheet.

@lkcampbell
Copy link
Contributor

@petetnt, never mind, I see what the problem is. I didn't realize that ugly Code Mirror word wrap hack made it into the core code. We really should have made that a preference.

Actually, it should not even be a preference. I think that functionality was available in an extension and it never worked correctly, even in the extension. It should not be in the core code IMHO. Sorry I didn't see it sooner or I would have shared my opinion sooner.

@petetnt
Copy link
Collaborator Author

petetnt commented Jan 29, 2016

FWIW I think the padding has been working pretty well (sans the 4px space issue this PR is trying to account for). I do think that the hack is pretty ugly too, but indent wrapping is much more crucial IMHO.

@petetnt
Copy link
Collaborator Author

petetnt commented Feb 5, 2016

In #12184 there's another case where the gutter shows up (and screws the fold marks too) with this fix. The thing is that I can't replicate it on Windows 10, 8.1 nor Mac OS, all with same version of Brackets Shell and same commit.

@abose
Copy link
Contributor

abose commented Apr 2, 2016

I too cant replicate the gap issue .

@ficristo
Copy link
Collaborator

LGTM, even I can not replicate the gap issue.

@redmunds
Copy link
Contributor

redmunds commented Jun 3, 2016

@abose Can we get this fix in for 1.7 ?

@petetnt
Copy link
Collaborator Author

petetnt commented Jun 3, 2016

@redmunds do you have any idea what causes the gutter thing yet? I have tried this on 10+ computers and I still haven't been able to replicate the issue you were having above 😕

@redmunds
Copy link
Contributor

redmunds commented Jun 3, 2016

@petetnt No, I haven't had time to look at that. But, I think it's an improvement.

@@ -27,6 +27,7 @@

> * {
text-indent: 0px;
margin-left: -4px;
Copy link
Contributor

Choose a reason for hiding this comment

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

@petetnt could this be done as part of the hack itself as opposed to stylesheet? this way the value of the shift (4px) will be shared and not duplicated.

Copy link
Contributor

Choose a reason for hiding this comment

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

@petetnt Not sure how to fix it in the hack itself, but I found out that just removing padding: 0 @code-padding 0 0; on line 26 also fixes the issue.

@abose
Copy link
Contributor

abose commented Jun 4, 2016

+1 this has been a long pending issue that's a must fix. Tagging @swmitra
also +1 It will be neater to club the hacks together in one place as @busykai pointed out.
I am guessing DW will also suffer from this selection issue if this bug is not addressed.

@petetnt
Copy link
Collaborator Author

petetnt commented Jun 24, 2016

Made some changes to this, 3ff302e now contains the compensation in the padding hack itself. Not sure if it's the most optimal solution either but seems to work for me.

@petetnt
Copy link
Collaborator Author

petetnt commented Jun 24, 2016

Build failed to the cla-check so disregard that.

@RaymondLim
Copy link
Contributor

@petetnt I confirmed that your fix is good with the default "show line numbers" on. If you turn off line numbers, then all lines are shifted to the left and your change does not fix in that case. So I believe we also need to adjust @code-padding in brackets_codemirror_override.less file to get it right.

@petetnt
Copy link
Collaborator Author

petetnt commented Jun 25, 2016

Thanks for the feedback @RaymondLim , I'll get that fixed ASAP 👍

@petetnt
Copy link
Collaborator Author

petetnt commented Jul 29, 2016

Somewhat related: #12630

@petetnt petetnt force-pushed the petetnt/indent-wrap-minor-fix branch from 3ff302e to 966b466 Compare July 31, 2016 17:43
@petetnt petetnt changed the title Compensate for the indent-wrap hack Remove basePadding from indentwrap hack Jul 31, 2016
@petetnt
Copy link
Collaborator Author

petetnt commented Jul 31, 2016

@RaymondLim adjusting @code-padding to 0 showed just similar issues for me (albeit upside down from the screenshots in this thread 🍔.

However, removing the basePadding variable altogether from the hack seemed to fix everything, as it is not really needed for the hack itself (Brackets pads itself with @code-padding from top of the editor and the left side of the pres).

Does it work for you / others?

@RaymondLim
Copy link
Contributor

@petetnt Sorry, I was wrong to say that I don't have any other changes. Actually, I do have the following change in Editor.js that I believe is from your previous commit.

        // For word wrap. Code adapted from https://codemirror.net/demo/indentwrap.html#
        this._codeMirror.on("renderLine", function (cm, line, elt) {
            var charWidth = self._codeMirror.defaultCharWidth(), basePadding = 4;
            var off = CodeMirror.countColumn(line.text, null, cm.getOption("tabSize")) * charWidth;
            elt.style.textIndent = "-" + off + "px";
            elt.style.paddingLeft = (basePadding + off) + "px";
            $(elt).children().each(function (i, child) {
                child.style.marginLeft = -basePadding + "px";
            });
        });

@petetnt
Copy link
Collaborator Author

petetnt commented Jul 31, 2016

Yeah, but that is not needed anymore. Together with #12630 all of the issues should be resolved. (The PR's solve different issues)

@ficristo ficristo added this to the Release 1.8 milestone Aug 2, 2016
@ficristo ficristo merged commit bf9b07d into adobe:master Aug 20, 2016
@ficristo
Copy link
Collaborator

LGTM, if we'll find more issue we can look at them in another PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants