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

fix handling of text scaling #182

Merged
merged 1 commit into from May 3, 2014
Merged

fix handling of text scaling #182

merged 1 commit into from May 3, 2014

Conversation

byorgey
Copy link
Member

@byorgey byorgey commented May 2, 2014

Fixes #179. This patch is very much along the lines outlined in
comments on the ticket. There will be accompanying patches to backends, so maybe we should wait to merge until we have all the backend fixes in place.

Fixes #179.  This patch is very much along the lines outlined in
comments on the ticket.  There will be accompanying patches to backends.
byorgey added a commit to diagrams/diagrams-cairo that referenced this pull request May 2, 2014
@jeffreyrosenbluth
Copy link
Member

This may be a non-trivial change for SVG as we don't carry around the accumulated style like we do in cairo?

@byorgey
Copy link
Member Author

byorgey commented May 2, 2014

Drat, you're right. I will ponder it a bit.

@byorgey
Copy link
Member Author

byorgey commented May 3, 2014

Curses, this is really annoying. The options seem to be (1) implement something similar to splitFillTextures that preprocesses the RTree, pushing FontSize attributues down and recording the info about Local vs non-Local in Text objects (ugh), (2) completely rewrite the SVG backend to track the current font size using a monad (ugh), or (3) wait until we implement fixpoint semantics (ugh).

@jeffreyrosenbluth
Copy link
Member

(1) Given the way you describe splitFillTextures " ... horrific mess that
is splitFills." I give this an ugh^2.
(2) From an implementation standpoint, this may not be that bad. We have a State monad that keeps track of clipping path ids and texture ids. I don't think we can just swap this out for a StateStack and add in accumulated style because we would get overlapping ids. But we only use State to generate id's, we could replace this fairly easily with a hash function for textures and possibly for clip paths by using the segTree. Then we wouldn't need to keep track of these counters. Moreover, if we are really worried about collisions, than we can actually string together the full record (at least for gradients).

On the other hand refactoring SVG in this way just to handle text doesn't feel like good design.

(3) I'm not as upset about this option, it gives as another reason to think seriously about implementing fix point semantics. We can revert back to the way text is handled in 1.1, i.e. not make font size of type Measure R2 but leave it as a Double until then. While from a aesthetic point of view it is really nice to have text units consistent with the rest of the library, from a practical point of view, text just won't be that useful in diagrams anyway until we implement a proper labeling api.

@byorgey
Copy link
Member Author

byorgey commented May 3, 2014

Oh! Wait, I honestly forgot we already used a state monad for SVG. I think it may not actually be too hard. I'm taking another look at it now.

@fryguybob
Copy link
Member

For (2) don't we need something like that if we are going to add sharing by
binding common structures to a name in SVG and referencing that name?

On Sat, May 3, 2014 at 1:50 PM, Jeffrey Rosenbluth <notifications@github.com

wrote:

(1) Given the way you describe splitFillTextures " ... horrific mess that
is splitFills." I give this an ugh^2.
(2) From an implementation standpoint, this may not be that bad. We have a
State monad that keeps track of clipping path ids and texture ids. I
don't think we can just swap this out for a StateStack and add in
accumulated style because we would get overlapping ids. But we only use
State to generate id's, we could replace this fairly easily with a hash
function for textures and possibly for clip paths by using the segTree.
Then we wouldn't need to keep track of these counters. Moreover, if we are
really worried about collisions, than we can actually string together the
full record (at least for gradients).

On the other hand refactoring SVG in this way just to handle text doesn't
feel like good design.

(3) I'm not as upset about this option, it gives as another reason to
think seriously about implementing fix point semantics. We can revert back
to the way text is handled in 1.1, i.e. not make font size of type Measure
R2 but leave it as a Double until then. While from a aesthetic point of
view it is really nice to have text units consistent with the rest of the
library, from a practical point of view, text just won't be that useful in
diagrams anyway until we implement a proper labeling api.


Reply to this email directly or view it on GitHubhttps://github.com//pull/182#issuecomment-42111688
.

@byorgey
Copy link
Member Author

byorgey commented May 3, 2014

Yes, but I don't see what all of this has to do with fixing text scaling.

@jeffreyrosenbluth
Copy link
Member

Nothing really. If we wind up needing both State and StateStack (which we may not), then it is an alternative to adding a monad transformer layer.

@fryguybob
Copy link
Member

@byorgey I was just speaking to @jeffreyrosenbluth's comment:

On the other hand refactoring SVG in this way just to handle text doesn't feel like good design.

@jeffreyrosenbluth
Copy link
Member

Ah, I was wondering what you (@fryguybob) where referring to.

On Sat, May 3, 2014 at 2:14 PM, Ryan Yates notifications@github.com wrote:

@byorgey https://github.com/byorgey I was just speaking to
@jeffreyrosenbluth https://github.com/jeffreyrosenbluth's comment:

On the other hand refactoring SVG in this way just to handle text doesn't
feel like good design.

Reply to this email directly or view it on GitHubhttps://github.com//pull/182#issuecomment-42112322
.

@byorgey
Copy link
Member Author

byorgey commented May 3, 2014

I don't think we really need StateStack---for the moment I have just sort of hacked it manually by saving the old Local boolean, setting the new one, then restoring the old after recursing. For this small case it's not too bad. Though there does still seem to be a bug so maybe I shouldn't speak too quickly.

byorgey added a commit to diagrams/diagrams-svg that referenced this pull request May 3, 2014
@byorgey
Copy link
Member Author

byorgey commented May 3, 2014

It wasn't that bad after all. Whew.

byorgey added a commit to diagrams/diagrams-rasterific that referenced this pull request May 3, 2014
@byorgey
Copy link
Member Author

byorgey commented May 3, 2014

This should be ready to merge now. The only issue is with the postscript backend, which would actually be annoying to fix in the way I thought the SVG backend would be.

jeffreyrosenbluth added a commit that referenced this pull request May 3, 2014
fix handling of text scaling
@jeffreyrosenbluth jeffreyrosenbluth merged commit 46154b2 into master May 3, 2014
@jeffreyrosenbluth jeffreyrosenbluth deleted the text-scale branch May 3, 2014 19:55
@mgsloan
Copy link
Member

mgsloan commented May 4, 2014

Hello all! I saw these changes, and figured I'd mention that some other code changes might be warranted:

Do these changes affect the Diagrams.Backend.Cairo.Text module?

Maybe some documentation should be added there to explain how to "properly" do this with measures / the fixpoint semantics. Should these operations even still be around? They are more efficient than doing multiple operations, but they can be error-prone, when the extra style argument is incorrectly supplied.

Can the fixpoint semantics measures stuff eliminate the need for the Style argument?

@byorgey
Copy link
Member Author

byorgey commented May 4, 2014

Ah, good call. Though I'm not sure any code changes are needed... I don't even think there's currently any way for those to work properly for anything other than a Local font size (the default). It may be possible with a fixpoint semantics (except that the IO gives me the heebie-jeebies). As for whether fixpoint semantics could eliminate the need for a Style argument, I am not sure. Can you remind me what the Style argument is for in the first place?

@bergey
Copy link
Member

bergey commented May 17, 2014

Empirically, Cairo.Text only works with Output font size; otherwise I get

textLineBounded: fromOutput: Cannot pass Normalized to backends, must be Output.

I need to think more about why this is, and what the behavior should be.

@byorgey
Copy link
Member Author

byorgey commented May 18, 2014

The relevant call chain goes like this: textLineBounded -> textLineBoundedIO -> getExtents -> cairoWithStyle -> cairoStyle. Essentially, textLineBounded takes a Style value and sneaks it to the cairo backend without going through the normal channels, so there is never any chance for the Style to have its Measure values converted to Output. But indeed, there is no way they could be at this point since we don't know the final size of the diagram, requested output size, etc. This is why I said above that there is no way for this to work properly with anything other than Local. I didn't realize it would actually cause a crash but I am not terribly surprised, nor can I think of anything reasonable it should do instead.

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 does not scale
5 participants