sublimehq / sublime_text

Issue tracker for Sublime Text
https://www.sublimetext.com
807 stars 39 forks source link

QuickPanelItem description labels are clipped, row height too small. #3591

Closed deathaxe closed 4 years ago

deathaxe commented 4 years ago

Description

The last line of the new QuickPanelItem's description label is clipped. It appears quick_panel_row height calculation assumes either hardcoded font sizes or doesn't take custom font sizes into account correctly. Depending on font face and size descriptions are more or less clipped.

Steps to reproduce

  1. Run Package Control: Install Package
  2. Install Theme - DAneo
  3. Select Theme -> DAneo - Origin
  4. Create Packages/User/DAneo - Origin.sublime-theme and paste the snippets below to manipulate font settings.

Expected behavior

The description text of a QuickPanelItem should display without clipping. Spacing above and below the labels should be equal.

Actual behavior

  1. The description text of a QuickPanelItem is clipped at the bottom. All letters like q, p, g are clipped below the baseline. (Red markers). Happens for both, labels and links.

  2. The selection rectangle doesn't apear to be vertical symetric, if clipping appears. The spacing below the description (quick_panel_path_label) seems quite a bit smaller then above the quick_panel_label. Maybe related?

  3. The default without any manipulation to the installed theme (font_face: system and quick_panel_path_font_size: 10):

    grafik

  4. The results with adjusting font_face and font_size of quick_panal_path_label:

    grafik

    {
       "variables": {
           "font_face": "iosevka light extra-extended",
           "font_size": 11,
    
           "quick_panel_path_font_size": 8.5,
       },
       "rules": [
       ],
    }
  5. A somewhat larger font_size:

    grafik

    {
       "variables": {
           "font_face": "iosevka light extra-extended",
           "font_size": 11,
    
           "quick_panel_path_font_size": 11,
       },
       "rules": [
       ],
    }
  6. With default system font

    grafik

    {
       "variables": {
           // "font_face": "iosevka light extra-extended",
           // "font_size": 11,
    
           "quick_panel_path_font_size": "var(font_size)",
       },
       "rules": [
       ],
    }
  7. Last but not least a screenshot with Default.sublime-theme, which seems to suffer from the same issue.

    grafik

Environment

deathaxe commented 4 years ago

I just found item_container class being assigned to those new quick panel items, which may be related with this bug.

The good news:

Adding the following rule to DAneo Base seems to fix the issue py properly adding the required padding to the bottom of those items so links can be rendered well.

        {
            "class": "item_container",
            "content_margin": [0, 0, 0, 2]
        },

font: iosevka

grafik

font: system

grafik

Or i just add kind_info so that symbol_container is used.

grafik

The bad news:

  1. I am getting crazy with all the different quick panel item types. Some use symbol_container, others item_container, most don't use any of them. Depending on whether I add kind_info or not the one or the other is used, Mixing quick_panel_row and mini_quick_panel_row, ... - goto symbol, goto anything, command palette, API quick panels, ... .

    Sorry, but htf should keep overview about which overlay uses which structure when? I find it very hard to follow the structure which feels a bit like a mess making theme development hard(er). It feels very defragmented, atm.

    Please get rid of symbol_container and consistently use item_container for all kinds of quick panel items, those with and those without kind_info. Maybe even the older ones?

    The differend kinds and needs to adjust the paddings is painful.

  2. While default theme suggests to set row_padding to 0 for quick panel items with kind info in order to adjust padding completely by kind_container and symbol_container ...

        {
            "class": "quick_panel",
            "parents": [
                {"class": "overlay_control kind_info"}
            ],
            "row_padding": [0, 0, 0, 0]
        },
    
        {
            "class": "symbol_container",
            "content_margin": [3, 7, 12, 7]
        },

    ... this is not possible for quick panel items which use item_container as there is no way to find out which type of quick_panel uses it. Trying something like ...

        {
            "class": "quick_panel",
            "parents": [
                {"class": "overlay_control"}
            ],
            "row_padding": [0, 0, 0, 0]
        },
    
        {
            "class": "item_container",
            "content_margin": [3, 7, 12, 7]
        },

    ... breaks normal/legacy quick panels (command palette, goto anywhere, ...).

deathaxe commented 4 years ago

Another thing I just realized is the height of quick panel items not being recaluclated after a theme change. Thus modified content_margins of item_container or symbol_container are applied after ST restart, while font size changes are applied immediatelly. This might have caused the heavily clipped fonts illustrated in the initial post (as I didn't restart ST after each change).

wbond commented 4 years ago

This bug was a result of the table control used by the quick panel not clearing the cached row size. If you restarted Sublime Text after changing a rule in the theme, no clipping would occur.

deathaxe commented 4 years ago

That's what I realized today, too, yes.

wbond commented 4 years ago

I fixed it earlier today, and it will be part of the next build.

deathaxe commented 4 years ago

Nice.

deathaxe commented 4 years ago

Would be nice to find a consistent way to control paddings for all the different quick panel items, too. Would it make sense to use item_container for all kinds of quick panel items (at least those which do not use symbol_container already) in order to be able to just set row_padding: 0 in general and just use the new containers?

wbond commented 4 years ago

The reason for symbol_container is so you can add padding only when kind info is displayed. This is necessary, otherwise the primary text in a row butts up against the kind container. The item_container was just an alternative class name for when kind info wasn't present, although I explicitly didn't document it because it is not intended to be used. Unfortunately with the ability to peek into the control structure, I can no longer keep implementation details hidden.

wbond commented 4 years ago

It may be that I need to bite the bullet and implement a custom control property for explicitly the space between the kind container and the text to the right of it. More or less I was trying to save myself having to write a single-use control with extra property specifiers and whatnot.

deathaxe commented 4 years ago

I don't want to need to use kind_container and I am with you about the kind info - symbol - spacing topic, yes. The less rules we need to to make all quick panel items kinds look consistent, the better it is.

I just found it hard to ensure padding below the details label to be correct, but maybe I was joking myself, due to the item height not being updated without restart.

deathaxe commented 4 years ago

grafik

  1. Removed, item_container,
  2. restarted ST.

I'd expect the padding below the details label to be the same as above the main label. But it isn't. It seems smaller.

*Control Tree

control tree at: 513.5,161.5
title_bar [file_medium_dark] pos: 0,0 size: 1920,1137
    .bg=[27, 29, 31, 255]
    .fg=[176, 177, 177, 255]
 edit_window window [file_medium_dark] pos: 0,0 size: 1920,1137
  grid_layout_control pos: 310,0 size: 1610,581
      .border_color=[35, 36, 38, 255]
      .border_size=1
   pane_container_control pos: 310,0 size: 815,581
    control pos: 417,46 size: 600,213
     overlay_control pos: 417,46 size: 600,213
         .content_margin=[16, 16, 16, 31]
         .layer0.inner_margin=[24, 19, 24, 34]
         .layer0.opacity=1
         .layer0.texture=res://Packages/Theme - DAneo/textures/overlay/overlay_shadow--mt10.png
         .layer1.inner_margin=[20, 16, 20, 32]
         .layer1.opacity=1
         .layer1.texture=res://Packages/Theme - DAneo/textures/overlay/overlay--mt10--bw0--br4.png
         .layer1.tint=[40, 42, 44, 255]
         .layer2.draw_center=false
         .layer2.inner_margin=[20, 16, 20, 32]
         .layer2.opacity=1
         .layer2.texture=res://Packages/Theme - DAneo/textures/overlay/overlay--mt10--bw1--br4.png
         .layer2.tint=[25, 26, 28, 255]
      filter_window pos: 433,62 size: 568,166
       scroll_area_control pos: 433,120 size: 568,108
           .overlay=true
        control pos: 433,120 size: 568,108
         quick_panel pos: 433,120 size: 568,108
             .dark_content=true
             .row_padding=[12, 7, 12, 7]
             .layer0.tint=[40, 42, 44, 255]
          quick_panel_row [selected] pos: 433,120 size: 568,54
              .layer0.opacity=1
              .layer0.tint=[255, 255, 255, 13]
           control pos: 445,127 size: 544,40
            item_container pos: 445,127 size: 544,40
             quick_panel_path_label quick_panel_detail_label pos: 449,158 size: 132,11
                 .fg=[255, 255, 255, 153]
                 .selected_fg=[255, 255, 255, 166]
                 .fg=[255, 255, 255, 153]
                 .font.face=iosevka light extra-extended
                 .font.size=9
                 .link_color=[123, 168, 249, 179]
    sheet_container_control pos: 310,46 size: 815,535
        .layer0.opacity=1
     scroll_area_control [scrollable, hscrollable] pos: 310,46 size: 815,535
         .overlay=true
      control pos: 310,46 size: 741,535
       text_area_control [file_medium_dark] pos: 310,-15294 size: 1071,110966
wbond commented 4 years ago

You should definitely not need to use padding to get things to display - that should only be for adding real padding.

The control structure for quick panel and auto complete items (they use the same base control now) is somewhat complex, and I agree the theme properties don't feel simple. Sometimes I try to hide the complexity via the documentation, but with the ability to introspect it makes it harder, and thus makes it harder to change things without themes breaking.

I think removing the ability to control top, right and bottom padding from symbol_container and finding a way to explicitly control the spacing between the kind container and the text would be better long term, and probably less confusing. Any top, right or bottom padding on symbol container should really be row padding, IMO.

deathaxe commented 4 years ago

The main issue with row_padding and kind_info is just the ability to tint the background of kind_info without gaps. That's what's broken by row_padding, which makes me wonder, what a good consistent strategy/structure was, which would work for all kinds of items.

wbond commented 4 years ago

I'd expect the padding below the details label to be the same as above the main label. But it isn't. It seems smaller.

I'd have to download the theme and dig into this some, but it is entirely possible the line height of the primary text is contributing to this blank space. I've seen such things before, especially on Windows. I know Consolas has a very low baseline. If you've got repeated lines of it, it doesn't seem off, but once you mix it with other things it can look wrong.

Setting a background color on the container or the row may help in diagnosing.

wbond commented 4 years ago

The main issue with row_padding and kind_info is just the ability to tint the background of kind_info without gaps.

That's a good point that I had forgotten about.

Part of the issue is that you almost certainly want the padding to be different to the left of the symbol/item when the kind info is present or absent. Otherwise you'll have either a large indent when the kind info is missing, or text up against the kind info container when it is present.

We could do item_container and item_container symbol, but either way we are going to need two rules. I suppose this form would be a little more consistent with the other "variants" we use in the theme class names.

deathaxe commented 4 years ago
  1. Added

    
        {
            "class": "item_container",
            "layer0.tint": "green",
            "layer0.opacity": 1.0,
        },
  2. Restart ST

Looks like the item_container is not heigh enough with iosevka font.

grafik

Same with "system" font (Segoe UI??)

grafik

Same with Adaptive.sublime-theme

grafik

The padding issue is not too obvious though due to small values.

wbond commented 4 years ago

I don’t have my Windows machine in front of me, hence why I am asking, but no worries if you don’t want to keep trying things.

Two thoughts:

deathaxe commented 4 years ago

Adaptive with restart etc.: Descenders are clipped.

    {
        "class": "item_container",
        "layer0.tint": "green",
        "layer0.opacity": 1.0,
    },

ST 4085

grafik

ST 4084 (with the same Data dir)

grafik


Adding content_margin ...

    {
        "class": "item_container",
        "content_margin": [0, 0, 0, 3],
        "layer0.tint": "green",
        "layer0.opacity": 1.0,
    },

fixes the issue:

ST 4085

grafik

ST 4084

grafik

wbond commented 4 years ago

Thanks for checking - I'll work on reproducing on my end.

wbond commented 4 years ago

The clipping is fixed in build 4086. I haven't looked into the line height issue yet.

deathaxe commented 4 years ago

This is what it looks like with ST 4086 on Windows 10 without using item_container.

grafik

I guess it's just because the line height is not sufficient.

wbond commented 4 years ago

This ended up being Windows-specific, related to a discrepancy between calculated line height and child control dimensions. It will be fixed in the next build.

wbond commented 4 years ago

This should be fully fixed in 4087

deathaxe commented 4 years ago

Beautiful.