Conversation
ControlsExample now uses @entry
# Conflicts: # Sources/SwiftCrossUI/Environment/EnvironmentValues.swift # Sources/SwiftCrossUI/State/AppStorage.swift # Sources/SwiftCrossUIMacrosPlugin/EntryMacro.swift # Sources/SwiftCrossUIMacrosPlugin/Utils/VariableExtension.swift # Tests/SwiftCrossUITests/EntryMacroTests.swift
Examples/Bundler.toml
Outdated
| product = 'ColorsExample' | ||
| version = '0.1.0' | ||
|
|
||
| [apps.LineLimitTest] |
There was a problem hiding this comment.
As you mentioned in the PR description, this should be removed before merging
| attributes: Self.attributes(forTextIn: environment) | ||
| ) | ||
|
|
||
| var usedHeight = rect.size.height |
There was a problem hiding this comment.
Rename this to height (as you later set it to not be the used height, but rather the height imposed by SCUI).
| var usedHeight = rect.size.height | ||
|
|
||
| if let lineLimitSettings = environment.lineLimitSettings { | ||
| let height = |
There was a problem hiding this comment.
Rename this to limitedHeight to not clash with the other variable rename
| /// Whether user interaction is enabled. Set by ``View/disabled(_:)``. | ||
| @Entry public var isEnabled: Bool = true | ||
|
|
||
| @Entry public var lineLimitSettings: LineLimit? |
| @@ -1,4 +1,4 @@ | |||
| struct StateImpl<Storage: StateStorageProtocol> { | |||
| public struct StateImpl<Storage: StateStorageProtocol> { | |||
There was a problem hiding this comment.
Make these internal again unless necessary (I think this may just be from a merging issue given that you changed this in an earlier pr?)
There was a problem hiding this comment.
yes removing the public keyword again worked without any warnings, curious that it was still here tho, I’m pretty sure the commit removing it was newer. anyway, its gone again
| context: nil | ||
| ) | ||
|
|
||
| var usedHeight = size.height |
There was a problem hiding this comment.
Similar to AppKitBackend, rename this var to height, and rename height (in the if) to limitedHeight
Sources/GtkBackend/GtkBackend.swift
Outdated
| ) | ||
|
|
||
| let pango = Pango(for: measurementCustomLabel) | ||
| let (_, potentialHeight) = pango.getTextSize( |
There was a problem hiding this comment.
Rename potentialHeight to heightLimit
| var usedHeight = height | ||
|
|
||
| if let lineLimitSettings = environment.lineLimitSettings { | ||
| let multilineString = [String](repeating: "a", count: lineLimitSettings.limit) |
There was a problem hiding this comment.
Is it possible for us to instead just measure a single "a" and then multiply the resulting height by the line limit? It'd be worth trying if you haven't already because it should be more efficient (don't need to construct a string and don't have to lay out as much).
There was a problem hiding this comment.
It is not, as gtk currently doesn’t respect the lineHeight.
the height of a single line of “a” * 2 is not the same as “a\na” due to some spacing(or smth like that) in between lines.
Once lineHeight is properly respected it would be a yes.
There was a problem hiding this comment.
Note that the actual baseline-to-baseline distance between lines of text is influenced by other factors, such as pango_layout_set_spacing() and pango_layout_set_line_spacing().
Just found that in the get_line_height docs. Might help resolve that
There was a problem hiding this comment.
doesn’t work
public func getLineHeight(_ text: String) -> Double {
let layout = pango_layout_new(pangoContext)!
pango_layout_set_text(layout, text, Int32(text.utf8.count))
let line = pango_layout_get_line(layout, 0)
var height: Int32 = 0
pango_layout_line_get_height(line, &height)
g_object_unref(UnsafeMutableRawPointer(layout))
return pango_units_to_double(height)
}using this * lineLimit leads to

(expected would be 1, 2, 3, 4)
There was a problem hiding this comment.
Ah I didn’t mean to use that function, because that would just return what the line height was set to. But the docs mention ‘baseline to baseline distance’ which is what SwiftCrossUI refers to as line height. They say that you need to incorporate the pango spacing and line spacing in order to compute that distance we want.
There was a problem hiding this comment.
should we just pango_layout_set_spacing for text in general to 0 to match the other platforms? And later allow custom line spacing via a https://developer.apple.com/documentation/swiftui/view/linespacing(_:)
There was a problem hiding this comment.
gtk documentation says linespacing and spacing are 0 by default. huh?
why doesn't it match then?
There was a problem hiding this comment.
Weird… might be worth double checking just in case
There was a problem hiding this comment.
Can confirm, both are 0
Sources/GtkBackend/GtkBackend.swift
Outdated
| multilineString, | ||
| ellipsize: (widget as! CustomLabel).ellipsize, | ||
| proposedWidth: proposedWidth.map(Double.init), | ||
| proposedHeight: proposedHeight.map(Double.init) |
There was a problem hiding this comment.
Have you tested this fully on Linux? It seems that reservesSpace = true would not function correctly due to the text getting ellipsized. You should set proposedWidth and proposedHeight to nil instead because we just want to know the ideal size of the measurement string (we don't want line wrapping or ellipsizing to happen).
There was a problem hiding this comment.
True, with proposedWidth/Height it can get squished. Setting to nil prevents it like you expected. And yes, I forgot test this case on GtkBackend, I’m sorry.
Still something wrong with the alignment, as it shouldn't be centered. Any idea how to prevent it? My brain isn’t really functioning well today
There was a problem hiding this comment.
vertical alignment now works after settign textView.yalign = 0.0 in the create function
There was a problem hiding this comment.
No worries about not testing it, it’s easy to miss stuff when you have to test it lots of times during development
Sources/GtkBackend/GtkBackend.swift
Outdated
| ) | ||
| return SIMD2(width, height) | ||
|
|
||
| var usedHeight = height |
There was a problem hiding this comment.
Maybe rename to imposedHeight or something similar. To me usedHeight sounds like the height that the text used, rather than the height that we are going to use as our return value (which I assume is what you meant?)
Resolves #432
Summary
Adds the
View/lineLimitmodifier, allowing to limit the vertical space a Text is allowed to occupy based on the amount of lines instead of fixed pixel values.Support:
Changes
View/lineLimitmodifierEnvironment/lineLimitSettingscontaining the lineLimit number and wether space should be reservedEnvironment/lineLimitcomputedInt?to match SwiftUI’s counterpart for usage in@Environment, setting is not supported, as I see clearness issues when it gets set withoutLineLimit/reservesSpaceImprovements
WinUI doesn’t require the single line height measurement anymore, as it now follows the SCUI dictated lineHeight of resolvedFont
Problems
On GtkBackend there is the issue that we can’t set the lineHeight, therefore its not immediately possible to calculate the height of Text if it had n lines.
I took the approach WinUI used before of just measuring another widget:
I create a multiline string with as many lines as lineLimit specifies and measure it inside a dedicated measurement widget that gets created at the start of the apps main loop.
Updating the widget and measuring it only happens when lineLimit is set in the environment, therefore not affecting performance unless used. lineLimit should also mostly get used with smaller numbers, so generating the string shouldn’t have too much of a performance impact
There are further issues when
Textis rendered inside aScrollView, detailed in #438Notes
The changes can be tested using
Examples/LineLimitTest, I see no point in keeping it around, so it should be removed before merging.This PR is built on top of #430
It should be merged first. This one is marked as draft until then.