Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Multiple font size / line height spans (updated) #203

Closed
wants to merge 40 commits into from

Conversation

nicoburns
Copy link
Contributor

@nicoburns nicoburns commented Nov 17, 2023

Implement specifying multiple font sizes and/or line heights.

Current status

Almost done. Working, with some fixes for the editing required.

Todo

  • fix scrolling
  • implement enum LineHeight
  • cache line_heights()
  • retire Metrics
  • rebase main merged into branch. Will need to squash prior to merge.
  • migrate tests
  • migrate benches
  • migrate example terminal (NOTE: does not compile on Windows with termion, see Update terminal example using olored` #129)
  • migrate example editor-orbclient
    • fix bug empty buffer does not render cursor
    • fix bug empty buffer has default Attrs
    • support rescaling font sizes on demand, perhaps via mutable access to spans of Attrs
  • migrate example editor-test
  • migrate example editor-libcosmic (NOTE: does not compile on Windows with pinned version of iced (incompatible API))
  • migrate vi feature
  • fix editor action Vertical
  • fix some editor actions not redrawing
  • tidy up
  • docs and PR write-up

Future work:

  • Split Attrs: color_opt, metadata, font_size, line_height
  • Buffer should have a default Attrs
  • Convenience method for updating all spans of Attrs in a Buffer
  • Handle start-of-line Attrs more carefully

Questions:

  • Should the Hash impl of Attrs include line height?
  • Should Metrics be restored as a property of Attrs, with definition Metrics { font_size: f32, line_height: LineHeight }?

@nicoburns nicoburns changed the title WIP Multiple font size / line height spans (rebased) WIP: Multiple font size / line height spans (rebased) Nov 17, 2023
@nicoburns nicoburns changed the title WIP: Multiple font size / line height spans (rebased) WIP: Multiple font size / line height spans (updated) Nov 17, 2023
@nicoburns nicoburns marked this pull request as draft November 17, 2023 18:52
@TotalKrill
Copy link
Contributor

whats missing in this? :)

@nicoburns
Copy link
Contributor Author

whats missing in this? :)

The checkbox list in the description is accurate. The main thing is the editor abstraction isn't quite working properly (compare for example pressing enter a bunch of times in the editor-libcosmic example on main vs. this PR - on main you get many blank new lines and the cursor always renders, on this PR you only get one newline and the cursor disappears).

If you have time to look into this your help would be very welcome. I've done some basic fixing up of compile errors. But haven't had time to look into the bugs.

@TotalKrill
Copy link
Contributor

TotalKrill commented Nov 24, 2023

Did a quick check, the bug with no cursor on newllne, is because the height of empy lines becomes zero from the LayoutLine::line_height() return the default value of f32 when the line contains no characters...

I was planning on changing it into returning an option, so empty lines return a None value, but I am unsure of where to grab the line height value from then... Probably the height of the previous lines last character... but that also mucks up if the previous line has no characters...

@TotalKrill
Copy link
Contributor

And there is also an error in the [Buffer:set_rich_text] code, its trying to add only newlines to the rich_text example makes that one bug out as well

…e the height of the line by providing an empty_height value to the layout function, that sets the line height for lines without characters
@TotalKrill
Copy link
Contributor

I dont like the segregation, so an update to this PR is found here

src/attrs.rs Outdated
/// # Panics
///
/// Will panic if font size is zero.
pub fn size(mut self, size: f32) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn size(mut self, size: f32) -> Self {
pub fn font_size(mut self, size: f32) -> Self {

@Dimchikkk
Copy link
Contributor

I've tested nicoburns:multi-size-fix-examples and TotalKrill:multi-size-fix-examples using my bevy plugin: https://github.com/StaffEngineer/bevy_cosmic_edit/tree/new-cosmic

Seems like there is a bug after rebasing tigregalis:multi-size... basically calling set_text with "Blah \n blah" creates buffer with 2 buffer lines as expected but attrs_list.spans calculated incorrectly for those lines.

@TotalKrill
Copy link
Contributor

TotalKrill commented Dec 10, 2023

I am currently testing this using the ./rich-text.sh in this repo. Could you elaborate on the issue you are seeing and would it be reproducable in this repository?

@Dimchikkk
Copy link
Contributor

@TotalKrill there is actually the issue on your fork. I created reproducible example here: TotalKrill/cosmic-text@multi-size-fix-examples...StaffEngineer:cosmic-text:multi-size-fix-examples

If you run rich-text example it prints:

[examples/rich-text/src/main.rs:58] line.text() = "Blah"
[examples/rich-text/src/main.rs:59] line.attrs_list() = AttrsList {
    defaults: AttrsOwned {
        color_opt: None,
        family_owned: Name(
            "Times New Roman",
        ),
        stretch: Normal,
        style: Normal,
        weight: Weight(
            400,
        ),
        metadata: 0,
        font_size: 54.34,
        line_height: Absolute(
            74.7175,
        ),
    },
    spans: {},
}
[examples/rich-text/src/main.rs:58] line.text() = "blah"
[examples/rich-text/src/main.rs:59] line.attrs_list() = AttrsList {
    defaults: AttrsOwned {
        color_opt: None,
        family_owned: Name(
            "Times New Roman",
        ),
        stretch: Normal,
        style: Normal,
        weight: Weight(
            400,
        ),
        metadata: 0,
        font_size: 54.34,
        line_height: Absolute(
            74.7175,
        ),
    },
    spans: {
        0..9: AttrsOwned {
            color_opt: None,
            family_owned: Name(
                "Times New Roman",
            ),
            stretch: Normal,
            style: Normal,
            weight: Weight(
                400,
            ),
            metadata: 0,
            font_size: 54.34,
            line_height: Absolute(
                74.7175,
            ),
        },
    },
}

But it should output:

[examples/rich-text/src/main.rs:58] line.text() = "Blah"
[examples/rich-text/src/main.rs:59] line.attrs_list() = AttrsList {
    defaults: AttrsOwned {
        color_opt: None,
        family_owned: Name(
            "Times New Roman",
        ),
        stretch: Normal,
        style: Normal,
        weight: Weight(
            400,
        ),
        metadata: 0,
        font_size: 54.34,
        line_height: Absolute(
            74.7175,
        ),
    },
    spans: {},
}
[examples/rich-text/src/main.rs:58] line.text() = "blah"
[examples/rich-text/src/main.rs:59] line.attrs_list() = AttrsList {
    defaults: AttrsOwned {
        color_opt: None,
        family_owned: Name(
            "Times New Roman",
        ),
        stretch: Normal,
        style: Normal,
        weight: Weight(
            400,
        ),
        metadata: 0,
        font_size: 54.34,
        line_height: Absolute(
            74.7175,
        ),
    },
    spans: {},
}

(notice spans are empty)

@TotalKrill
Copy link
Contributor

TotalKrill commented Dec 11, 2023

Updated with a fix to that, I believe :), thanks for helping testing!

@nicoburns nicoburns mentioned this pull request Feb 19, 2024
19 tasks
@nicoburns
Copy link
Contributor Author

Closing in favour of #235 (which is this PR rebased on top of latest main)

@nicoburns nicoburns closed this Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spans of font sizes
5 participants