Repository navigation
Conversation
allow plotting plot and legend to two different picts
944e54c to
8b2c0cf
Compare
alex-hhh
left a comment
alex-hhh
left a comment
There was a problem hiding this comment.
I have added some practical commends about the pull request, and I will merge it once they are addressed, however, I am a concerned about the entire idea of providing the plot legend as a picture.
I know this was requested in the original issue, but the forum discussions requested just placing the legend outside the plot area, and the idea of having the legend as a pict was the idea of a single person and I feel it was not discussed enough.
I am not sure that anyone would use this feature, so it is very difficult to determine if we provided the right thing. And we have limited opportunity to correct things once they are published in a Racket release, as the plot package needs to be backwards compatible forever.
However, placing the legend outside the plot area is a feature that is available in other plot libraries and we can be more certain that we provided the right (or at least a workable) interface...
| (define legend (get-legend-entry-list renderer-list bounds-rect)) | ||
|
|
||
| (define-values (width height) | ||
| (parameterize/group ([plot-parameters saved-values]) |
There was a problem hiding this comment.
I don't think we need parameterize/group here -- this code runs in the same thread that captured the parameters.
| [#:x-min (U Real #f) #:x-max (U Real #f) | ||
| #:y-min (U Real #f) #:y-max (U Real #f)] | ||
| Pict)) | ||
| (define (legend-pict renderer-tree |
There was a problem hiding this comment.
The name legend-pict is a bit confusing (in my opinion anyway). I think plot-legend-as-pict would be a better option, but perhaps there are other possibilities?
I also feel that using the x-min ... arguments is a bit counter-intuitive, but I understand why it is necessary.
| (send pd restore-drawing-params))) | ||
| width height)])) | ||
|
|
||
| (:: plot+legend-picts |
There was a problem hiding this comment.
I don't think this function is necessary, as it does not provide any more functionality than calling plot-pict and legend-pict separately, and I would prefer to keep the new API functions to a minimum
| [#:x-min (U Real #f) #:x-max (U Real #f) | ||
| #:y-min (U Real #f) #:y-max (U Real #f)] | ||
| Pict)) | ||
| (define (legend3d-pict renderer-tree |
There was a problem hiding this comment.
Same remark about the naming, perhaps plot3d-legend-as-pict?
| #:x-label (U String pict #f) | ||
| #:y-label (U String pict #f)] | ||
| (Values Pict Pict))) | ||
| (define (plot+legend3d-picts renderer-tree |
There was a problem hiding this comment.
Same remark as for 2D plots -- I don't think this function is necessary
|
I don't mind not introducing this, as I prefer to have the legend in the plot. But if we do, I think the better option is the one where plot+legend are provided, so at least the |
|
Unless someone has a concrete use case for this, as opposed to just brainstorming some possible use cases, we can easily add it in, given all the refactoring that was done. However, once we add something to the plot package API, it will need to be maintained practically forever, so I am hesitant to add things for "just in case" scenarios, because we might implement them to support only limited use cases so they have the potential to be useless to most of the users. Also note, that users can also create manual legends using picts, although perhaps a bit inconvenient, but with a lot of flexibilities, this was illustrated in the google-groups discussion listed from #49. |
|
I'd like to try this change, but the branch has been deleted and the I could copy/paste the code manually, but I'd rather avoid as I expect complications. Can the branch be restored, or is there another way I can patch this? |
|
I deleted this branch on all my devices. It seems github gives me an option to "restore branch", but I don't know if it will work. |
|
Quickly: I could eventually patch it on top of HEAD (sorry, just saw you restored the branch), and it works fine with I'll get back to this soon, hopefully. Thanks for starting this! |
allow plotting plot and legend to two different picts as was the original question in #49