Skip to content

-[MMCoreTextView initWithFrame:] doesn't initialize fontDescent #883

Closed
#927
Closed
-[MMCoreTextView initWithFrame:] doesn't initialize fontDescent#883
#927
Labels
10.15 Catalina
Milestone
snapshot-159

Description

@jjgod
jjgod
opened on Apr 22, 2019

-[MMCoreTextView initWithFrame:] initializes font but not fontDescent:

// NOTE!  It does not matter which font is set here, Vim will set its
// own font on startup anyway.  Just set some bogus values.
font = [[NSFont userFixedPitchFontOfSize:0] retain];

so it defaults to 0. In the case of the later -setFont: is called with the same pointer as font, then other initialization related to the font won't happen.

- (void)setFont:(NSFont *)newFont
{
    if (!(newFont && font != newFont))
        return;

There are many ways to fix this, a simple way would be just to call [self setFont:[NSFont userFixedPitchFontOfSize:0]] at -initWithFrame:.

Activity

  1. jjgod commented on Jul 26, 2019

    @jjgod
    jjgod
    on Jul 26, 2019
    ContributorAuthor

    Any update?

  2. ychin commented on Jul 27, 2019

    @ychin
    ychin
    on Jul 27, 2019
    Member

    I can take a look. Do you have a concrete repro that I can use to validate this? As in what are the symptoms form this issue?

  3. jjgod commented on Jul 27, 2019

    @jjgod
    jjgod
    on Jul 27, 2019 via email
    ContributorAuthor

  4. eirnym commented on Jul 28, 2019

    @eirnym
    eirnym
    on Jul 28, 2019
    Contributor

    Just launch MacVim without any .vimrc/.gvimrc on macOS 10.15 beta you will see this, all the text will be clipped.
    …

    this depends on a font

  5. jjgod commented on Jul 28, 2019

    @jjgod
    jjgod
    on Jul 28, 2019 via email
    ContributorAuthor

  6. eirnym commented on Jul 28, 2019

    @eirnym
    eirnym
    on Jul 28, 2019
    Contributor

    I mean the issue with clipping is font-specific. When I set to Menlo, the issue is clearly present, when I set it to Fira Code, clipping is not present. These conditions and outcomes are repeatable even you mix the order.

  7. jjgod commented on Jul 28, 2019

    @jjgod
    jjgod
    on Jul 28, 2019 via email
    ContributorAuthor

  8. jjgod commented on Jul 28, 2019

    @jjgod
    jjgod
    on Jul 28, 2019
    ContributorAuthor

    Most fonts will have a non-zero descender. When fontDescent is not initialized properly this will defintiely be a problem.

  9. eirnym commented on Jul 28, 2019

    @eirnym
    eirnym
    on Jul 28, 2019
    Contributor

    So you want to say, that guifont setting sets fontDescent to some meaningful value? If yes, why setting Menlo after Fira Code Light I able to observe clipping?

  10. jjgod commented on Jul 28, 2019

    @jjgod
    jjgod
    on Jul 28, 2019
    ContributorAuthor

    I'm sorry, exactly what you do to trigger this? Do you have steps to reproduce this behavior?

    As far as I can tell,

    https://github.com/macvim-dev/macvim/blob/master/src/MacVim/MMCoreTextView.m#L327

    fontDescent = ceil(CTFontGetDescent(fontRef));

    Since it always assigns fontDescent here it shouldn't be a problem as long as -setFont: is being called properly every time a guifont changes.

  11. jjgod
    added a commit that references this issue on Jul 28, 2019

    -[MMCoreTextView initWithFrame:] doesn't initialize fontDescent

    92dd6ba
  12. ychin
    added this to the snapshot-158 milestone on Jul 29, 2019
  13. ychin
    added
    10.15 Catalina
    on Jul 29, 2019

  14. ychin commented on Jul 29, 2019

    @ychin
    ychin
    on Jul 29, 2019
    Member

    If I'm guessing correctly, this happens in 10.15 because macOS is reusing and caching NSFont objects internally. Previously, we just naively assume setFont will always be called (which is true), and then the font will be different from the initial dummy font. However, it appears that macOS 10.15 will just pass the same NSFont to us (if your vimrc doesn't override the default font), causing the first setFont to early-out and not set up fontDescent properly. I commented on your pull request for further improvements before I want to merge it in.

  15. jjgod commented on Jul 29, 2019

    @jjgod
    jjgod
    on Jul 29, 2019
    ContributorAuthor

    Correct. PR updated.

  16. ychin
    closed this as completedin #927on Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    10.15 Catalina

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions