779642 - Scratchpad Line numbers don't line up

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P2)

Description

[Rob Campbell [:rc] (:robcee)]

Regression introduced with the landing of bug 756423. On Mac, the line numbers no longer line up due to the additional height of the command key on Macs.

Comment 1

[Rob Campbell [:rc] (:robcee)]

Attachment: scratchpad screenshot

Comment 2

[Rob Campbell [:rc] (:robcee)]

One possible solution for this would be to change the string returned for the Command Key to "Command" or "Cmd".

Comment 3

[Mihai Sucan [:msucan]]

Ouch. This happens because Orion, at initialization, checks the line height and it assumes all chars have the same height. Unfortunately the Command Key character is of a different height, breaking a lot of things in Orion (not just line numbers alignment).

Good suggestion - we should just say "Command" or "Cmd".

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

So if the user types or pastes this (or a similar) character, Orion will lose it again? Would using the height of this character (or a similar "big" one) for initialization work?

Comment 5

[Mihai Sucan [:msucan]]

I noticed some fonts have different "big" characters. we can't know which is the biggest char in each font. The actual solution is to fix how Orion works internally. Afaik, they will allow variable line heights in the future.

Additionally, if you don't use any of the big chars in your lines but you determine the line height using a big char during init.... you still have the same problem.

Comment 6

[Rob Campbell [:rc] (:robcee)]

looks like mihai is going to file upstream. We should do the keystring fix mentioned above in the meantime.

Filter on BLACKEAGLE?!

Comment 7

[Mihai Sucan [:msucan]]

Reported upstream:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=386517

Comment 8

[Thaddee Tyl [:espadrine]]

Changing "⌘" to "Cmd" wouldn't solve the problem, since the user can type this character.
On the other hand, changing the font we use to Menlo, which is the monospaced font that
Apple wants everybody to have as a default anyway, would fix it for good.
Besides, it is a great font.

Comment 9

[Thaddee Tyl [:espadrine]]

Attachment: Using the Menlo font fixes the issue.

For some reason, using this fix makes my font look big.
Does anyone have the same issue?

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 649048 [details] [diff] [review]
Using the Menlo font fixes the issue.

unfortunately, we can't do that. We should respect the system's font-settings. I would personally love to use Menlo, but if a user picks a different font...

Comment 11

[Rob Campbell [:rc] (:robcee)]

and, fwiw, my own system has Menlo specified here.

Comment 12

[Mark Capella [:capella]]

I just noticed this.

Until orion ever opts to go for variable line height, I'd like to change this to use "Cmd" to at least fix the regression caused by my patch.

This still follows Mac convention, and if the user inserts a cloverleaf char into the text, the problem is of his own doing, and is an exposure that currently exists on the orion side / not of our doing.

Comment 13

[Rob Campbell [:rc] (:robcee)]

(In reply to Mark Capella [:capella] from comment #12)
> I just noticed this.
> 
> Until orion ever opts to go for variable line height, I'd like to change
> this to use "Cmd" to at least fix the regression caused by my patch.
> 
> This still follows Mac convention, and if the user inserts a cloverleaf char
> into the text, the problem is of his own doing, and is an exposure that
> currently exists on the orion side / not of our doing.

yeah, I'd like that until we get a proper fix for line-heights. I think that's the way to go. Feel like taking it?

Comment 14

[Mark Capella [:capella]]

Attachment: Patch (v1)

Temp Fix - Override Mac |cloverleaf| char with |Cmd-| string, until Orion fixes variable line height.

Comment 15

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 654863 [details] [diff] [review]
Patch (v1)

+      // Use "Cmd-" literal vs cloverleaf meta-key until
+      // Orion adds variable height lines
+      //elemString += keysbundle.GetStringFromName("VK_META_CMD") +
+      //              keysbundle.GetStringFromName("MODIFIER_SEPARATOR");

could you reference this bug in the comment? You can preface with a XXX bug 779642 on your first comment line. Also add a space between the comment // and elemString.

r+ with those nits fixed.

Comment 16

[Mark Capella [:capella]]

Attachment: Patch (v2)

As pushed to TRY...
https://tbpl.mozilla.org/?tree=Try&rev=3b2d1b5509ad

Comment 17

[Mark Capella [:capella]]

And on to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=98a07ac71ddc

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98a07ac71ddc

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

We need to fix this in Aurora as well, right?

Comment 20

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 655794 [details] [diff] [review]
Patch (v2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 756423
User impact if declined: users's line numbers won't line up in the scratchpad.
Testing completed (on m-c, etc.): on m-c, try
Risk to taking this patch (and alternatives if risky): users won't like the scratchpad.
String or UUID changes made by this patch: none (internal, non l10n strings only)

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 655794 [details] [diff] [review]
Patch (v2)

[Triage Comment]
New feature, early in the cycle, low risk fix. Approved for Aurora 17.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/66731d17d130

Comment 23

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0

Verified with Ubuntu, Mac OS 10.7 and Windows 7. No problems with rendering and lining up characters with line numbers if the apple key character isn't used.

Comment 24

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old