DTAttributedTextContentView setFrame behavior not normal #102

Closed
gshaviv opened this Issue Jan 9, 2012 · 21 comments

Comments

Projects
None yet
7 participants
Contributor

gshaviv commented Jan 9, 2012

I have a code where I determine the size needed for the DT...View and then position it, e.g:

CGSize sz = [aView suggestedFrameSizeToFitEntireStringConstraintedToWidth:neededWidth];
aView.frame = CGRectMake(someX,someY,sz.width,sz.height);
NSLog(@"aView.h=%.0f sz.h=%.0f",aView.frame.size.height,sz.size.height);

I find that on many occasions there is a few pixel difference between the height I set the view to the height it gets. This causes several issues:

  • It's not standard UIKit behavior, a view should get the frame size it is assigned.
  • It screws up fine layout algorithms. e.g. I want the bottom of the view frame to be at a certain Y but the bottom often is not there even though I calculated it to be there since the re-position algorithm of the view is always to preserve the top-left.
  • Why does suggestedFrameSizeToFitEntireStringConstraintedToWidth: not return the accurate height needed?

My workaround is to set the view frame to the needed width and infinite height, then reposition it based on what frame size it actually got. But this is a hack and shouldn't be needed.

Collaborator

odrobnik commented Jan 9, 2012

At present the UIView gets resized as soon as the layouting has been done. I will implement a property that allows you to steer the autoresizing behavior:

  • no resizing
  • adjust height
  • adjust width
  • adjust both
Contributor

gshaviv commented Jan 9, 2012

But there is also the issue that the suggested size method does not
return the right size, i.e. the size the view resizes itself to.

Guy

On 9 בינו 2012, at 12:36, Oliver Drobnik
reply@reply.github.com
wrote:

At present the UIView gets resized as soon as the layouting has been done. I will implement a property that allows you to steer the autoresizing behavior:

  • no resizing
  • adjust height
  • adjust width
  • adjust both

Reply to this email directly or view it on GitHub:
#102 (comment)

kcoop commented Feb 22, 2012

Any progress on this? I'm having the same issue as gshaviv: the sizeThatFits: method on DTAttributedTextContentView does not conform to the intended behavior. In particular, because layout occurs at the time the attributedString is set, sizeThatFits: does not pay any attention to the width in calculating the height - it just uses the precalculated value.

In order for this to work in the context of other generic hierarchical layout code, the view can't depend on the width being specified up front. It would be better to have sizing done in a separate pass from layout, with appropriate caching of intermediate results.

kcoop commented Feb 22, 2012

I should also say I appreciate all the work you've put into this. It is not a small effort, and very useful!

I see your use cases have this as a root view. If you want it to play well in a hierarchy of views (which is my situation, and I imagine that of many others), you really can't overload the use of the view's frame to save layout state. That way lies madness.

kcoop commented Feb 23, 2012

As for the inaccuracy of suggested size, this may shed some light on the issue:

http://stackoverflow.com/questions/3374591/ctframesettersuggestframesizewithconstraints-sometimes-returns-incorrect-size

And while the following comment in the code suggests the problem is isolated to large documents pre iOS 5:
// attributedStringSizeThatFits: returns an unreliable measure prior to 4.2 for very long documents.

I am still seeing it now, in very short documents on iOS 5.

kcoop commented Feb 23, 2012

This is a total hack, but for anyone needing accurate measurement in the near future, here's a solution that works for me (I don't recommend it for large bodies of text):

kcoop/DTCoreText@9e4f305

Collaborator

odrobnik commented Feb 23, 2012

Yes, total hack. But the only way that works. I'll probably implement something like this for all those madmen who don't understand that attributed strings don't work like NSStrings.

kcoop commented Feb 23, 2012

While expressed as a quick bandaid, the idea is proper. You could optionally maintain a small cache of these results, keyed by the constraining width, and use matches in your actual layout. That would be more efficient for handling a measurement phase, as often the constraint widths and the resulting frame widths are the same.

And you really do need a measurement phase, for any case where views need to follow the rich text. There's some solace in the notion that for these cases, the amount of text involved is not typically huge.

gschom commented Aug 11, 2012

Did anything ever come of this? I don't want my rich text content views to resize automatically. If the content's height is bigger than a certain value then I want to truncate the rest.

reflog commented Oct 22, 2012

bump. this is a real problem... kcoop solution works but only for a subset of cases... for some, it still returns invalid height. any suggestions? the thing that bugs me the most - relayoutText does set the correct frame (the [super setFrame] line) but it doesn't match what's returned in the [suggestedSize...] method.

hsoi commented Oct 29, 2012

It sounds like this is the problem I'm presently chasing down as well.

In my case, I'm using a DTAttributedTextCell. The -tableView:heightForRowAtIndexPath: simply returns the [cell requiredRowHeightInTableView:], and it was frustrating to see the top of my text 5 pixels inset from the cell's bounds but the bottom flush. Having DCIntrospect hooked into my app I could see the bottom edge of the DTAttributedTextContentView was extending -6 beyond the bottom bounds. And when I'd see the requiredRowHeight returning 50 but DCIntrospect telling me it was 55... frustrating. Finally tracked it down to the same as other mentioned: that -[DTAttributedTextCell layoutSubviews] calls -[DTAttributedContentView setFrame:] which eventually calls -relayoutText, and the -sizeThatFits: ends up returning the 55 and changing the frame of the DTAttributedTextContentView in a manner not consistent with the value returned by -requiredRowHeightInTableView:.

@Cocoanetics - Is there anything more you need from us to help find resolve to this issue?

Collaborator

odrobnik commented Oct 30, 2012

@hsoi you are welcome to fix it.

hsoi commented Oct 30, 2012

I figured that would be the response.

FWIW, using kcoop's workaround gets me by for now. If I get the time, I'll see about hunting this down more completely and providing a better fix.

Thank you for responding.

hsoi commented Jan 8, 2013

Finally had a chance to come back to this.

While I'm not sure of this being the solution to the problem, I think I have a better workaround that seems to be working across all cases I've tested so far. I wanted to share for whatever it might be worth.

The problem was that the -[UITableViewController tableView:heightForRowAtIndexPath:] was using the -[DTAttributedTextCell requiredRowHeightInTableView:] , and that was having a different height than the DTAttributedTextCell.attributedTextContextView.frame.size.height. Full details in my above comment.

So if that's the case, why not have the -heightForRowAtIndexPath: return the DTAttributedTextCell.attributedTextContextView.frame.size.height? Would make sense. But alas, it doesn't work because by the time the table gets the height, the contextView doesn't have a height, and by the time it has a height, the table doesn't get heights any more. So what to do?

Well, I changed -[DTAttributedTextCell layoutSubviews] so when/if the _attributedTextContextView.frame is being set (since that's where the recalcs happens and everything gets throw off), force the table to refetch the cell heights by this:

// only change frame if width has changed to avoid extra layouting
if (_attributedTextContextView.frame.size.width != frame.size.width)
{
    _attributedTextContextView.frame = frame;

    // added the following
    UITableView*    parentTable = (UITableView*)[self superview];
    [parentTable beginUpdates];
    [parentTable endUpdates];
}

and that works to kick the table and refetch the row heights, and everything seems to work out well. When inspecting the GUI with DCIntrospect, it all looks right and good. Many different HTML inputs, rotating the device, various sizes... whereas before it didn't take long before I had a test case that blew things out with other workarounds, so far everything here has worked with what I've thrown at it. YMMV.

Collaborator

odrobnik commented Jan 9, 2013

@hsoi ideally there should be no need to re-layout the content views. To achieve that the layout frame could be created based on a target size. Then when setting the text we would want the content view to not relayout itself. Calling a resize in the cell cannot be a good solution because it will always relayout multiple times.

Here are several issues combined in this issue:

  • how do I get a fitting frame size?
  • what should happen if the frame of the content view is changed?
  • what should happen if the attributed text of the content view is changed?
  • should the content view actually be accessible or should it be an internal view of the scroll view?
  • what's the most efficient (read: avoid multiple size changes) way to have non-fixed-height table view cells with attributed text?

The problem is that we are doing our own layout for several features and to workaround CoreText bugs. Because of this the suggestedFrameSize... Method is defunct because it relies on CoreText's faulty estimation fix. The only accurate method at present is to have a layout frame build the lines and then take the actual size needed.

Currently changing the text or frame size causes a relayout of the content view and those changes are then picked up by the scroll view via KVO to set the contentSize appropriately. This is why I proposed a size changing mask property as outlines above.

The big problem with variable height table view cells is performance on tables with many rows. This calls the heightForRow... for ALL cells. And having this occur means that because of the custom layouting we need to build the lines and glyph runs for all text in all cells. I know of production apps that use this albeit with extreme caching of the values. i.e. Remember the row height for a given tweet.

Maybe we could add such row height caching as well where the hash of the HTML string would be the cache key or a row height with certain options and constraint width. It is probably outside of the scope of the demo to provide such functionality.

The chicken-and-egg problem can be solved by the proposed relayouting mask.

hsoi commented Jan 9, 2013

Hi @Cocoanetics, and thank you for the reply.

I agree that this is NOT a good solution because it causes relayout. It does work (at least in my test cases and my particular use need), even if it's not optimal. I tend to prefer "correctness" over "speed", and right now this brings me (from an end-user perspective in my app), correctness, within the shipping deadlines I'm up against. I agree it's not ideal tho and would prefer a proper solution. And that's part of why I shared it here. While it might work for me, others are using DTCoreText (especially yourself) in far different ways and so you and others may be able to present other scenarios, which you have, and which make the solution even less optimal as a general-purpose solution. Plus it generates this discussion, which I appreciate.

I tried the approach of making a DTAttributedTextContentView with my string when the table asked for the row height, so I could make it once and be done with it when asked for, but alas, try as I might, I couldn't get it to work right in all cases.

But it sounds like you have an idea of a solution, be it a solution that gets implemented in the core library (e.g. the relayouting mask) or in the demo app (e.g. row height caching) or both. And while I grant you prior statement of me being welcome to fix this issue, it's evident you have the best grasp upon your own code and the deep complex issues involved in this matter. So I can hope that you might actually fix it, since you seem to be the best man for the job. :-) But if it's something that cannot be fixed, perhaps an FAQ or blog posting that details the issues, history, and suggestions for coping with the matter could be written up so others encountering this problem (since I'm obviously not the only one) could find ways to address it.

Thank you for your time.

Collaborator

odrobnik commented Jan 26, 2013

There is a new mode that somebody introduced called flexibleHeight. This only adjusts the height of the content view if set to YES. This might be a step towards the relayout mask.

Collaborator

odrobnik commented Jan 26, 2013

Actually I am considering an alternative route. The content view only changes its size so that the DTAttributedTextView knows about it.

People generally want to get the needed size first and then set the frame. Presently you would need to create a layout frame for a given width and open height and inquire about the needed size. Then the relayou would do the same all over again.

Instead we could cache layout frames based on size constraint and hash of the attributed string. This would only do the layout once per string and constraint size. Multiple calls to get a layout frame for a given size would return the previously cached frames.

This would allow total removal for self-resizing from the content view. Thoughts?

Love it.

@odrobnik odrobnik added a commit that referenced this issue Feb 5, 2013

@odrobnik odrobnik Added relayoutMask to contentView. Related to #102
This is used to prevent an automatic re-layout if the frame changes, e.g. if you control the frame size yourself after doing your own layout and you don#t want the view to change itself when setting the new frame.
bb8b488

@odrobnik odrobnik added a commit that referenced this issue Feb 7, 2013

@odrobnik odrobnik Added caching of generated layout frames
The goal of these changes is to have the DTCoreTextLayouter cache generated layout frames for a given string, range and frame. This way layout for e.g. DTAtt
ributedTextCells is only done once per width. i.e. twice for portrait and landscape. The same cached frame will also be used for the suggestedFrameSizeToFitEntireStringConstraintedToWidth of DTAttributedTextContentView.

In short suggestedFrameSizeToFitEntireStringConstraintedToWidth now creates a temporary rectangle which is the bounds inset by the edgeInsets and height set
 to open-ended. As long as you don't change the frame the same layoutFrame can be reused. Caching occurs in DTCoreTextLayouter, as generally one layouter = one attributed string.

This should fix #284 - not wrapping correctly for plain style. I changed the demo to use plain style and it wraps ok.
It should also fix #237 since the requiredRowHeightInTableView now always uses the same layoutFrame as the cell will use later on
Finally it also deals with one aspect of #102 as caching the layout frame allows for efficient getting of the needed frame.

The final piece to solving #102 is to remove the super.frame from -relayoutText in DTAttributedTextContentView. This method is called if:

- width or height changes and the matching relayoutMask is set
- a new attributedString is set
- new edge insets are set (note though that when using DTAttributedTextView now the scrollView contentInset is used instead

I imagine that the contentView might send a notification if relayoutText occurs to be able to adjust the contentSize and adjust the content view height to match. The second scenario would be to manually call sizeToFit if you want to get a view that matches its contents perfectly.
4ff275b

odrobnik closed this in 21e4bcb Feb 7, 2013

@jachenry jachenry added a commit to TackMobile/DTCoreText that referenced this issue Feb 20, 2013

@odrobnik @jachenry odrobnik + jachenry Added relayoutMask to contentView. Related to #102
This is used to prevent an automatic re-layout if the frame changes, e.g. if you control the frame size yourself after doing your own layout and you don#t want the view to change itself when setting the new frame.
99e3589

@jachenry jachenry added a commit to TackMobile/DTCoreText that referenced this issue Feb 20, 2013

@odrobnik @jachenry odrobnik + jachenry Added caching of generated layout frames
The goal of these changes is to have the DTCoreTextLayouter cache generated layout frames for a given string, range and frame. This way layout for e.g. DTAtt
ributedTextCells is only done once per width. i.e. twice for portrait and landscape. The same cached frame will also be used for the suggestedFrameSizeToFitEntireStringConstraintedToWidth of DTAttributedTextContentView.

In short suggestedFrameSizeToFitEntireStringConstraintedToWidth now creates a temporary rectangle which is the bounds inset by the edgeInsets and height set
 to open-ended. As long as you don't change the frame the same layoutFrame can be reused. Caching occurs in DTCoreTextLayouter, as generally one layouter = one attributed string.

This should fix #284 - not wrapping correctly for plain style. I changed the demo to use plain style and it wraps ok.
It should also fix #237 since the requiredRowHeightInTableView now always uses the same layoutFrame as the cell will use later on
Finally it also deals with one aspect of #102 as caching the layout frame allows for efficient getting of the needed frame.

The final piece to solving #102 is to remove the super.frame from -relayoutText in DTAttributedTextContentView. This method is called if:

- width or height changes and the matching relayoutMask is set
- a new attributedString is set
- new edge insets are set (note though that when using DTAttributedTextView now the scrollView contentInset is used instead

I imagine that the contentView might send a notification if relayoutText occurs to be able to adjust the contentSize and adjust the content view height to match. The second scenario would be to manually call sizeToFit if you want to get a view that matches its contents perfectly.
b7b99dd

@jachenry jachenry added a commit to TackMobile/DTCoreText that referenced this issue Feb 20, 2013

@odrobnik @jachenry odrobnik + jachenry remove self-resizing of content view
DTAttributedTextContentView no longer self-resizes due to layout changes. If you are using it directly without DTAttributedTextView (which adds scrolling) then you have to first set the attributedString and then you have two options to know the optimal frame:

- You have subscribed to DTAttributedTextContentViewDidFinishLayoutNotification which communicates the optimal frame rect in the OptimalFrame value of the userInfo
- You call sizeToFit. This gets the size from sizeThatFits: which in turn gets it from intrinsicContentSize.

closes #102
735aec4

@hsoi hsoi added a commit to barz/DTCoreText that referenced this issue Mar 17, 2014

@hsoi hsoi + John C. Daub Remove our DTAttributedTextCell workaround, as documented in Cocoanet…
…ics#102 . Instead, adopting the 'DTAttributedTextContentViewDidFinishLayoutNotification' approach that is mentioned in a commit later in that issue.
daa9d51

hsoi commented Mar 18, 2014

As a long-overdue follow up.... just in case this might help someone stumbling their way here via Google.

I never liked the workaround I came up with, but at the time was under shipping pressure and it got us by. Unfortunately it had a fugly side-effect of doing this "growing height animation" of the table cell every time things (re)drew. It was live-able, no one complained... but it was a constant reminder to me of the less-than-optimal workaround.

In bringing up the app on iPad, I had a rare and difficult to track crasher. I managed to track it down to this workaround, because when the device would rotate and I guess the table layout could be "just right" and the -beginUpdate/-endUpdate while trying to update cells got things deep in the bowels of the OS all confused and assertions would fire.

Instead, I adopted @Cocoanetics approach of the DTAttributedTextContentViewDidFinishLayoutNotification. I had the UIViewController that hosted my table view listen for this notification; when it came in [self.tableView reloadData]. However, that itself wasn't enough because the act of doing so would cause the notification to fire again, and endless looping would result. So since the premise of things now is to calculate once (maybe twice) then cache and (re)use that result, I had my UIViewController do the same thing: store off the height of the cell; if my stored height wasn't the same as the notification's height, -reloadData else do nothing. Have to kick things at least once to get the layout fully looking right, but this is so much better than before. Works like a charm. No more multiple redraws, no more animating height, and everything is properly sized. Much better solution.

Thank you (again), @Cocoanetics :-)

Collaborator

odrobnik commented Mar 18, 2014

@hsoi you're welcome! I just released 1.6.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment