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

Rich text spans #1589

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Rich text spans #1589

wants to merge 1 commit into from

Conversation

0HyperCube
Copy link
Member

@0HyperCube 0HyperCube commented Jan 28, 2024

Closes #1105

Featuring:

A comment from Discord:

[Ths] experimental code to deal with the spans was quite hacky and probably has some edge cases I hadn't considered. In addition, features such as the letter spacing are applied after cosmic-text does the line wrapping as I couldn't see a way to factor that in, leading to the line wrapping not working properly with letter spacing.

@Keavon
Copy link
Member

Keavon commented Feb 18, 2024

Sorry it's taken me so long to get to this! I have a first round of QA feedback that it'd be most awesome if you could help polish it up for being production-ready.

  • ~Editing text should ideally not occur as an outline (nor its selection background). I realise it's using the overlays system, but perhaps you could take a stab at making it mostly match the styling (at least the fill colour, maybe even fill gradient and stroke colour if you're extra ambitious).~~

    No as this is just abusing the overlay system for things it is not meant to do i.e. rendering the document.

  • Similarly to above, since it's reusing the overlays system, it would be helpful to make the text rendering be immune to having the Overlays checkbox hide the overlays. Maybe generalise this, because there will likely be other kinds of overlays in the future that we want to show at all times.

    No as this is just abusing the overlay system for things it is not meant to do i.e. rendering the document.

  • When the Text tool is active, it's very easy to click on text and not actually begin editing that text but instead place down new text. This could be made clearer by highlighting the text on hover (similar to hover with the Select tool active) to give useful visual feedback to know you're indeed targeting it for editing.

    Can be implemented later.

  • To further improve the previous point about difficulty in selecting text to edit (either with the Text tool single-click or Select tool double-click), would it be possible to expand the click target area for each character? Maybe something like using the bounding box of each character, or some rectangular approximation for each entire line of text. Ideally, for example, clicking on a space between words should be a valid click target to begin editing where that space is in the text.

    Not implementing as the select tool does not know which layer is text and where the glyph bounds are without copying all of the cosmic text stuff.

  • Fix bug breaking the text on path

  • In the Properties panel, "Line Width" being set to the max float is a bit confusing. Are you able to make that a radio button that toggles between "Auto Width" and "Auto Height"? And when set to Fixed Width, it would show a second row for the width (with the addition of a " px" unit).

    No I don't feel like adding some "auto height" as it is not clear what that would do.

  • It would also be most helpful to visualise the bounding box of the text, and when set to Auto Height, this bounding box would usually be wider than the widest row of text (but would fit the height automatically to all lines).

    I find this confusing and difficult to implement.

  • The Text tool would ideally be able to click to place text in "Auto Width" mode (like it does now), or click-and-drag the bounding box to place text with a certain line width in "Auto Height" mode. (The height of the bounding box would get ignored, but the width stored.)

    Could maybe implement in future?

  • Paper cut: it's difficult to double click inside small text, while using the Select tool, because the click target area for the resizing edges of the transform cage get in the way. Could you please make it so only the outsides of the transform cage are eligible for being dragged? (For everything, not just text.)

    could do.

  • Once you're done with the testing, switching back from "text italic bold l e t t e r s p a c i n g word spacing" to either blank, "Lorem ipsum", or "Text" as the default text would be best for a polished user experience.

    The user experience is fine.

Sorry if that's an overwhelming bunch of things! Feel free to push back on any that you'd prefer to de-scope for follow-up work after an earlier merge. I was erring on the side of thoroughness for this first QA pass. Thanks :)

@Keavon Keavon marked this pull request as draft February 18, 2024 22:07
@0HyperCube 0HyperCube closed this Feb 19, 2024
@0HyperCube 0HyperCube changed the title Revamp text with rich styling Text spans Feb 25, 2024
@Keavon
Copy link
Member

Keavon commented Apr 2, 2024

Reopening since I'm planning to either make my requested changes above or enlist community contributors.

@Keavon Keavon reopened this Apr 2, 2024
@0HyperCube
Copy link
Member Author

@Keavon is it necessary to keep this open as it is accruing many merge conflicts? The code here is not very good anyway.

@Keavon
Copy link
Member

Keavon commented May 13, 2024

I'm happy to keep rebasing it, I think it provides us some value in having the option (when I get time or can assign it to a new contributor) to finish this up since even if it has some code issues it is probably still an upgrade over the current text situation, right? Although if you envision a better approach that's different from this, it might be helpful to know your perspective on that for the context about where this differs from what you'd consider the best route to go with it.

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.

Text tool improvements
2 participants