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

GlyphView is not a RendererView #5189

Merged
merged 21 commits into from Sep 30, 2016
Merged

GlyphView is not a RendererView #5189

merged 21 commits into from Sep 30, 2016

Conversation

@mattpap
Copy link
Contributor

@mattpap mattpap commented Sep 19, 2016

This originally started as webgl splitoff (hence mattpap/webgl floating around), but realized that I need to do some restructuring and cleanups and in end we got this. Some of those changes may seem unrelated, but if you look specifically PlotCanvas and GlyphView, you should see the point. You may also see some minor code duplication. This may be factored out in future using mixin composition. At least there is no fake code reuse as it was before. There is a lot of work still to be done to stabilize bokehjs' APIs, but this is a valuable first step. I tested every commit with gulp test and a few, fairly representative examples. However, there might be some breakage, so examples have to be verified more carefully.

fixes #5188

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Sep 19, 2016

Can we put new things in core? I'd actually like to get rid of common

@mattpap
Copy link
Contributor Author

@mattpap mattpap commented Sep 19, 2016

@bryevdv, yes, I wasn't sure myself where to put it. This core vs. common situation is confusing, so I will be happy do deal with it.

@@ -446,7 +438,7 @@ class PlotCanvasView extends Renderer.View

for tool_view in tool_views
level = tool_view.model.level
@levels[level][tool_view.model.id] = tool_view
@levels['tool'][tool_view.model.id] = tool_view

This comment has been minimized.

@ghost

ghost Sep 19, 2016

Is this right? The normal case is 'tool' but the intent was that in some cases, some tools might want to specify their level differently.

This comment has been minimized.

@mattpap

mattpap Sep 19, 2016
Author Contributor

@bigreddot, where this knowledge comes from? Tools shouldn't participate in render_levels() at all and this is just a temporary measure before proper implementation emerges. What you may be speaking about are tool introduced (so called, synthetic) annotations, which, as they are renderers, may belong to different levels of rendering (usually overlay or underlay). Originally those levels were defined on tools, but this was wrong and inconsistent with bokeh.models.

This comment has been minimized.

@ghost

ghost Sep 19, 2016

Actually thinking back, I think there was point where some tools did their own drawing, and were in fact renderers. That's not currently the case anymore. If we want to dictate that tools may have renderers, but not be renderers, that's OK.

This comment has been minimized.

@mattpap

mattpap Sep 20, 2016
Author Contributor

Yes, historically it might have been true, but tools were never meant to be renderers. This is annotations' (specifically overlays') job to do painting of crosshair lines, selection areas, etc. I went ahead and removed tool rendering level altogether to avoid confusion.

@mattpap mattpap force-pushed the mattpap/5188_glyphview branch from c4e4459 to b2a9e98 Sep 20, 2016
@bryevdv bryevdv added the MIGRATION label Sep 20, 2016
@mattpap
Copy link
Contributor Author

@mattpap mattpap commented Sep 28, 2016

Apparently, test failures (yes, all builds are deceptively green) are unrelated to this PR and must have been there for awhile, though at least texas example, the only shared between, works in PR #4490 (where LogColorMapper was added, which is the offending piece of code). The failure is undefined is not a function (evaluating Math.log1p(n)). Good luck figuring this out without either running this example manually (which actually may not fail, because it may be a browser related issue) or browsing image diff. And image diff is only helpful because of a feature I added recently in PR #4961. Also, Math.log1p() isn't available in IE. Perhaps it's not available in phantomjs as well.

@mattpap
Copy link
Contributor Author

@mattpap mattpap commented Sep 29, 2016

Issue with Math.log1p() was fixed in PR #5249.

@mattpap
Copy link
Contributor Author

@mattpap mattpap commented Sep 29, 2016

@bokeh/dev, I would like to merge this sooner than later due to possible merge conflicts. This also blocks my further work in this area. How we are standing on the release?

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Sep 29, 2016

We should merge and test then, the release will not happen this week

@mattpap mattpap merged commit d2f1132 into master Sep 30, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mattpap mattpap deleted the mattpap/5188_glyphview branch Sep 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.