[CLOSED] Use integer line-height to avoid rounding differences

[core-ai-bot]

Issue by redmunds
Thursday May 16, 2013 at 02:40 GMT
Originally opened as adobe/brackets#3849

---

Yet another attempt at #3478.

@iwehrman Can you try this branch to see if it fixes the issue for you?

---

redmunds included the following code: https://github.com/adobe/brackets/pull/3849/commits

[core-ai-bot]

Comment by iwehrman
Thursday May 16, 2013 at 16:14 GMT

---

Nailed it!

[core-ai-bot]

Comment by redmunds
Thursday May 16, 2013 at 16:52 GMT

---

@TomMalbran Care to review this one? I updated fix so it doesn't round off em values.

[core-ai-bot]

Comment by TomMalbran
Friday May 17, 2013 at 00:34 GMT

---

Sure I can.

This looks good and finally fixes the problem, but since we previously increased the line height ratio a lot to avoid rounding it made a big gap on a really big font size between the light backgrounds of 2 consecutive rows. Since we now need to round the value, maybe we could decrease the line height ratio a bit, 1.27 seems to work nicely on Win XP. But with 1.27 the fourth view commands integration test is failing, for a similar problem to the ones fixed. So this test would need to be fixed too.

[core-ai-bot]

Comment by redmunds
Friday May 17, 2013 at 00:41 GMT

---

I definitely want to fix any gaps, but a ratio of 1.27 seems too weird for me. I tried 1.25 with Math.round() but that didn't fix the original problem. Maybe 1.25 with Math.ceil() will work?

[core-ai-bot]

Comment by TomMalbran
Friday May 17, 2013 at 01:01 GMT

---

1.25 and ceil seems to be working on Windows... does it work on Mac too?

[core-ai-bot]

Comment by redmunds
Friday May 17, 2013 at 15:52 GMT

---

@TomMalbran 1.25 and ceil looks good on Mac, both retina and non-retina. I see some gaps if you make font-size super large, but that's not a common workflow.

Chnages pushed. Ready for re-review.

[core-ai-bot]

Comment by TomMalbran
Friday May 17, 2013 at 22:10 GMT

---

This looks great. I can see some gaps at some big font-sizes but is just a 1px gap, and I don't think we can do nothing about it, but it is a lot less than with the previews line height ratio.

Before merging this I see a failure in the fourth test in view command handlers. Do we need to fix that now or add a new issue for it? It has a similar problem to the other 2 tests, when we switched from 1px updates to line ratios ones. And there is a issue with Travis, with one of the tests, which I am not sure why it is here. Maybe a final merge with master could fix it?

[core-ai-bot]

Comment by redmunds
Friday May 17, 2013 at 22:38 GMT

---

Sorry, I forgot about the unit test. Pushed a fix.

[core-ai-bot]

Comment by peterflynn
Friday May 17, 2013 at 22:47 GMT

---

Fwiw -- I have a feeling we'll switch to a different way of highlighting Find results in a month or two anyway, so hopefully the gaps at larger sizes will eventually go away for free...

[core-ai-bot]

Comment by TomMalbran
Friday May 17, 2013 at 22:49 GMT

---

Great, tests are passing now. Merging.

[core-ai-bot]

Comment by redmunds
Friday May 17, 2013 at 22:58 GMT

---

Thanks,@TomMalbran !