Fix 1351 wrong nsstringdrawer truncating text #28

optimisme · 2023-04-13T07:07:41Z

This fix solves two problems:

String drawing performance is very slow when truncating large texts
String drawing does not properly truncate NSLineBreakByTruncatingHead and NSLineBreakByTruncatingMiddle

This pull request is related with reported bug:

That can be closed after the merging

bugaevc

Cool; some comments below. In general, please watch out for memory leaks and for spacing. alignment.

Yes, perhaps my preferred objc style is slightly unconventional; it's closer to GNUstep's than to Apple's. Where Apple would write:

-(void)forwardInvocation:(NSInvocation*)anInvocation;

GNUstep would write:

- (void) forwardInvocation: (NSInvocation*)anInvocation;

I would write:

- (void) forwardInvocation: (NSInvocation*) anInvocation;

Hope this makes sense!

bugaevc · 2023-04-13T13:35:26Z

AppKit/NSStringDrawer.m

+        [self _clipAndDrawInRect: rect
+                    lineBreakMode: NSLineBreakByTruncatingTail];
+    } else {
+        [self _clipAndDrawInRect: rect
+                    lineBreakMode: NSLineBreakByClipping];


Sorry in advance for nitpicking, but: code style!

Here in particular, the colons have to be vertically aligned:

Suggested change

[self _clipAndDrawInRect: rect

lineBreakMode: NSLineBreakByTruncatingTail];

} else {

[self _clipAndDrawInRect: rect

lineBreakMode: NSLineBreakByClipping];

[self _clipAndDrawInRect: rect

lineBreakMode: NSLineBreakByTruncatingTail];

} else {

[self _clipAndDrawInRect: rect

lineBreakMode: NSLineBreakByClipping];

bugaevc · 2023-04-13T13:38:00Z

AppKit/NSStringDrawer.m

+    [self _clipAndDrawInRect: rect truncatingTail: YES];
+}
+
+- (void)_clipAndDrawInRect:(NSRect)rect lineBreakMode:(NSLineBreakMode)lineBreakMode {


Suggested change

- (void)_clipAndDrawInRect:(NSRect)rect lineBreakMode:(NSLineBreakMode)lineBreakMode {

- (void) _clipAndDrawInRect: (NSRect) rect lineBreakMode: (NSLineBreakMode) lineBreakMode {

or probably even

Suggested change

- (void)_clipAndDrawInRect:(NSRect)rect lineBreakMode:(NSLineBreakMode)lineBreakMode {

- (void) _clipAndDrawInRect: (NSRect) rect

lineBreakMode: (NSLineBreakMode) lineBreakMode

{

bugaevc · 2023-04-13T13:39:34Z

AppKit/NSTableHeaderCell.m

    [[self attributedStringValue]
            _clipAndDrawInRect: [self titleRectForBounds: cellFrame]
-                truncatingTail: (_lineBreakMode > NSLineBreakByClipping)];
+                 lineBreakMode: _lineBreakMode];                


Suggested change

lineBreakMode: _lineBreakMode];

lineBreakMode: _lineBreakMode];

bugaevc · 2023-04-13T13:48:02Z

AppKit/NSStringDrawer.m

+        NSDictionary *attributes = [string attributesAtIndex:[string length] - 1 effectiveRange:NULL];
+        NSAttributedString *ellipsis = [[NSAttributedString alloc] initWithString:@"..." attributes:attributes];


Suggested change

NSDictionary *attributes = [string attributesAtIndex:[string length] - 1 effectiveRange:NULL];

NSAttributedString *ellipsis = [[NSAttributedString alloc] initWithString:@"..." attributes:attributes];

NSDictionary *attributes = [string attributesAtIndex: [string length] - 1

effectiveRange: NULL];

NSAttributedString *ellipsis = [[NSAttributedString alloc] initWithString: @"..."

attributes: attributes];

Also, you're leaking the ellipsis now, aren't you? It was autoreleased previously.

bugaevc · 2023-04-13T13:56:39Z

AppKit/NSStringDrawer.m

+        NSAttributedString *ellipsis = [[NSAttributedString alloc] initWithString:@"..." attributes:attributes];
+
+        NSInteger left = 0;
+        NSInteger right = [string length];


When writing binary search-like algorithms, I find it helpful to explicitly spell out the invariant. For instance:

// INVARIANT: The string truncated to 'left' characters always fits, // and truncated to 'right' characters always does not fit.

Note that your invariant seems to be different -- or is it? What's yours, exactly?

bugaevc · 2023-04-13T14:01:51Z

AppKit/NSStringDrawer.m

+        NSMutableAttributedString *tmpString;
+
+        while (left < right) {
+            mid = (left + right) / 2;


So I wonder if we could do better than the regular binary search here, by taking advantage of the fact that we're not only getting a boolean (fits / doesn't fit), but the actual width measurements. We could estimate mid based on the measurements. It'd still be confined between left and right, and we'd still be converging towards the answer, but hopefully we can do it in fewer steps.

For instance if we know that at left = 10 the string takes up 100 px, and at right = 20 it takes up 200px, and our room for the string is 110 px wide, mid = 11 would be a better estimate than 15. We only have to ensure that it's strictly greater than left and less than right, otherwise we'll loop endlessly.

I tried to do it this way but it didn't work and was very slow.

bugaevc · 2023-04-13T14:03:09Z

AppKit/NSStringDrawer.m

+            switch (lineBreakMode) {
+                case NSLineBreakByTruncatingHead:
+                    clippedTitle = [string attributedSubstringFromRange:NSMakeRange(mid, [string length] - mid)];
+                    tmpString = [[[NSAttributedString alloc] initWithAttributedString:ellipsis] mutableCopy];


You're leaking the pre-mutableCopy string too. Are you perhaps used to programming with ARC?

bugaevc · 2023-04-13T14:09:15Z

AppKit/NSStringDrawer.m

+
+                case NSLineBreakByTruncatingMiddle:
+                    clippedTitle = [string attributedSubstringFromRange:NSMakeRange(0, mid)];
+                    NSAttributedString *clippedTail = [string attributedSubstringFromRange:NSMakeRange([string length] - mid, mid)];


So here's an issue: if mid is high enough, you're going to start duplicating parts of the string, e.g. if string = @"Hello world" and mid is 7, you'd get clippedTitle = @"Hello w", clippedTail = @"o world", tmpString = @"Hello w...o world".

This new string mostly likely wouldn't fit (considering the original didn't, since we're inside this if), but better prevent this from happening in the first place, i.e. start from right = [string length] / 2 for the NSLineBreakByTruncatingMiddle mode.

optimisme · 2023-04-14T17:28:30Z

I have submitted a new version with some of the proposed changes. However, others, more complex, are beyond my knowledge."

optimisme · 2023-04-26T15:42:55Z

I merged the new AppKit code with this branch, is there a problem to merge now?

CKegel · 2023-05-01T19:06:43Z

I have submitted a new version with some of the proposed changes. However, others, more complex, are beyond my knowledge."

What changes do you think are beyond your knowledge? If you elaborate a bit I'm sure myself or someone else may be able to help! Don't forget about the discord server, as the learning channel is a great place to gain knowledge.

luzpaz · 2023-06-23T21:44:42Z

Any progress on this ?

CKegel · 2023-06-23T21:49:29Z

Any progress on this ?

Not that I have seen, the remaining issues brought up by the reviewer still need addressed, and the commits squashed. I'm happy to provide help in any way I can

optimisme added 4 commits April 11, 2023 20:02

Fix NSStringDrawer truncated texts and performance

57cc5f0

Fixed missing NSLineBreakByClipping

3bf0491

Fixed 1351

29fd280

Handle truncation cases separately

442cc7c

bugaevc reviewed Apr 13, 2023

View reviewed changes

Added contribution suggestions

6ee1944

Merge branch 'master' into fix-1351-wrong-nsstringdrawer-truncating-text

2b617ea

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 1351 wrong nsstringdrawer truncating text #28

Fix 1351 wrong nsstringdrawer truncating text #28

optimisme commented Apr 13, 2023

bugaevc left a comment

bugaevc Apr 13, 2023

bugaevc Apr 13, 2023

bugaevc Apr 13, 2023

bugaevc Apr 13, 2023

bugaevc Apr 13, 2023

bugaevc Apr 13, 2023

optimisme Apr 14, 2023

bugaevc Apr 13, 2023

bugaevc Apr 13, 2023

optimisme commented Apr 14, 2023

optimisme commented Apr 26, 2023

CKegel commented May 1, 2023

luzpaz commented Jun 23, 2023

CKegel commented Jun 23, 2023

	- (void)_clipAndDrawInRect:(NSRect)rect lineBreakMode:(NSLineBreakMode)lineBreakMode {
	- (void) _clipAndDrawInRect: (NSRect) rect lineBreakMode: (NSLineBreakMode) lineBreakMode {

-- (void)_clipAndDrawInRect:(NSRect)rect lineBreakMode:(NSLineBreakMode)lineBreakMode {
+- (void) _clipAndDrawInRect: (NSRect) rect
+              lineBreakMode: (NSLineBreakMode) lineBreakMode
+{

	lineBreakMode: _lineBreakMode];
	lineBreakMode: _lineBreakMode];

		NSDictionary *attributes = [string attributesAtIndex:[string length] - 1 effectiveRange:NULL];
		NSAttributedString *ellipsis = [[NSAttributedString alloc] initWithString:@"..." attributes:attributes];

-        NSDictionary *attributes = [string attributesAtIndex:[string length] - 1 effectiveRange:NULL];
-        NSAttributedString *ellipsis = [[NSAttributedString alloc] initWithString:@"..." attributes:attributes];
+        NSDictionary *attributes = [string attributesAtIndex: [string length] - 1
+                                              effectiveRange: NULL];
+        NSAttributedString *ellipsis = [[NSAttributedString alloc] initWithString: @"..."
+                                                                       attributes: attributes];

Fix 1351 wrong nsstringdrawer truncating text #28

Are you sure you want to change the base?

Fix 1351 wrong nsstringdrawer truncating text #28

Conversation

optimisme commented Apr 13, 2023

bugaevc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

optimisme commented Apr 14, 2023

optimisme commented Apr 26, 2023

CKegel commented May 1, 2023

luzpaz commented Jun 23, 2023

CKegel commented Jun 23, 2023