Text fixup #1235
Text fixup #1235
Conversation
| row_left += w | ||
| # Return the very end of the box if we can't figure it out. | ||
| return len(rows) - 1, len(rows[len(rows) - 1]) | ||
| # Start from left |
There was a problem hiding this comment.
I know this is probably a little more processor intensive, but it works, and it's silly to repeat a ton of the same code when you don't have to.
|
@dlonie care to review this one? |
| for display in self.displays: | ||
| for key in display.backend: | ||
| try: | ||
| if actor == display.backend[key] or actor in display.backend[key] or [True for group in display.backend[key] if actor in group]: |
There was a problem hiding this comment.
I'd suggest manually wrapping lines longer than 80 characters. The author is better at breaking lines into easy-to-parse logical chunks than a text editor -- this make it easier to spot errors in logic, etc and quickly grok the conditional parameters while reading through the code.
There was a problem hiding this comment.
I'm going to break this logic up a bit; I spotted what may be a small issue in it, so that will help tidy that up.
|
Looked through the patch, nothing major. One small issue with computing dimensions from vtkTextRenderer::GetBoundingBox that should be fixed, but the rest of the comments are more or less just suggestions. I haven't pulled/tested yet. Let me know if I should go ahead an create the new baselines from the VTK change if @doutriaux1 or @chaosphere2112 hasn't already put them together. |
|
@dlonie I have commits fixing all of the things you pointed out, just waiting on my ports to open so I can push them. I haven't updated the baselines yet; @doutriaux1 have you? |
|
@dlonie @doutriaux1 @aashish24 Pushed up the fixes. |
|
ok we got a passing build at: https://open.cdash.org/viewTest.php?onlyfailed&buildid=3790700 |
|
LGTM, a couple failures in that dashboard, but unrelated to this change. |
This pull request fixes a bunch of small interaction issues (enumerated below) with how I was handling text previously– among other things, it fixes #1205. It's dependent on this PR, which invalidates some baselines (ctest results) that I haven't regenerated yet; we can do those as part of merging the VTK PR.
The associated PR for testdata is here the changes in this PR invalidated some button baselines, so I've regenerated those in the PR.
Interaction issues:
Plus some more, I'm sure.
@dlonie If you can double check my text actor handling in text.py and textbox.py, that'd be great.
@aashish24 This is the PR that I mentioned in the meeting, @doutriaux1 and you can both feel free to poke through it.