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

Improve the layout subsystem #8085

Merged
merged 232 commits into from Feb 1, 2019

Conversation

Projects
None yet
@mattpap
Copy link
Contributor

commented Jul 17, 2018

addresses #8084

fixes #4407 (new feature)
fixes #4643 (irrelevant)
fixes #5169 (fixed, needs a test)
fixes #5572 (fixed)
fixes #6294 (fixed)
fixes #6417 (fixed, needs a test)
fixes #6768 (fixed, needs a test)
fixes #7120 (fixed, needs a test)
fixes #7185 (irrelevant)
fixes #7497 (fixed)
fixes #7590 (fixed) [already closed?]
fixes #7771 (fixed)
fixes #8227 (fixed)
fixes #8229 (new feature, needs tests, examples)
fixes #6822 (fixed)
fixes #4395 (fixed)

fixes #8391 (fixed, needs test)
fixes #7260 (fixed)
fixes #7454 (fixed, needs test)
fixes #5749 (fixed)
fixes #6259 (fixed)
fixes #4608 (fixed)
fixes #8611 (fixed)

@mattpap mattpap added the status: WIP label Jul 17, 2018

@mattpap mattpap force-pushed the mattpap/8084_layout branch from c6d3a60 to f6d0024 Jul 18, 2018

@mattpap mattpap force-pushed the mattpap/8084_layout branch 5 times, most recently from 151bda8 to 6358e3a Aug 8, 2018

@mattpap mattpap force-pushed the mattpap/8084_layout branch from 6358e3a to e3fd5d4 Aug 17, 2018

@bryevdv

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

@mattpap I'd like to suggest an alternative to deprecation for widgetbox. It's fairly common in other tools to be able to create a columns of widgets of fixed width, and perhaps with a different background, e.g. light grey, to offset the area of controls visually. I believe we could make widgetbox be that, i..e just a convenience function that creates a column but with a standard fixed width and a light grey background configured. It's not something we have to heavily emphasize or endorse, but it would be a little useful and also would let us avoid a deprecation in 1.0

@mattpap mattpap force-pushed the mattpap/8084_layout branch from e3fd5d4 to 899eea0 Aug 20, 2018

@mattpap

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2018

I'd like to suggest an alternative to deprecation for widgetbox.

I'm open to suggestions regarding this. One thing that I particularly dislike about widgetbox, is that it doesn't tell from its name what kind of layout to expect. I presume it makes sense it's a column, but nevertheless it's confusing to me.

@bryevdv

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

I'm open to suggestions regarding this.

Do you have thoughts on what I suggested above? I agree "widgetbox" is somewhat vague but I think visually distinguished boxes with vertical layouts are common enough that we could reasonably use it for that (and it would also be simple to implement I expect)

@mattpap

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2018

I will implement what was suggested.

@mattpap mattpap force-pushed the mattpap/8084_layout branch 4 times, most recently from 71c5fbd to e6c640e Aug 22, 2018

@bryevdv bryevdv referenced this pull request Aug 27, 2018

Merged

Bryanv/5231 json embeds #8193

3 of 3 tasks complete
@bryevdv

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

@mattpap a data point to keep in mind:

https://groups.google.com/a/continuum.io/forum/#!topic/bokeh/AGB2JEuRoSs

After some investigation I discovered that the issue was excessive calls to render on the data table, and that those calls to render were ultimately triggered by lots of re-layout-ing. I would definitely be nice if we can avoid things like that, do you think this work as it is may already help?


A quick run with the PR does have the panning much more responsive, though the data table is not really rendering yet (just the header). In general I guess it might be worth trying to optimize for the fact that unless a canvas size changes, any layout changes inside the canvas can't really affect any widget layouts. And perhaps that's already the case -- I don't see excessive call to render the table anymore on layout.

@mattpap

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

In this PR rendering happens exactly once, before the first layout. It's up to a model's view to decide if it needs to anything more after layout, so it can render again or just update its subcomponents based on possibly changed layout. This way you can control exactly what needs to be updated and how expensive it will be.

@mattpap mattpap force-pushed the mattpap/8084_layout branch from 14ffebd to 750f52c Sep 4, 2018

@philippjfr philippjfr referenced this pull request Sep 6, 2018

Merged

Compatibility for new bokeh layout system #32

8 of 9 tasks complete
@bryevdv

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

@mattpap you will probably want to merge/rebase on master due to #8219

@mattpap mattpap force-pushed the mattpap/8084_layout branch from 750f52c to e202d51 Sep 7, 2018

@philippjfr

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

@mattpap At what point is feedback here valuable to you? I've been trying out this PR every once in a while and have noticed a large number of regressions and backward incompatible changes. Obviously I wasn't going to raise those while you're still working on this, but if you're nearing completion I want to make sure these are either addressed or explicitly acknowledged as incompatibilities before this is merged.

@mattpap

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

@philippjfr, I'm presuming you are referring to pyviz/panel#32? So, widgets/tabs are still work in progress, so feedback will be welcome when things are fully implemented (hopefully today, but more realistically on Monday). bokehjs' APIs changed almost completely with regards to layout, so external models will have to be updated (perhaps pyviz/panel should be added to downstream tests, to avoid future regressions). I can do it when I'm done with this PR. There should be no API difference in Python, though there are some corner cases that have changed (e.g. sizing mode propagation in grids), however you shouldn't be affected by them. There are some regressions to updating things on change, especially when children change. I will be reviewing all listeners/emits when I'm done with the base implementation. I'm also expecting a "tweaking" period after this PR is done and possibly merged, to get all the corner cases right (and there are a lot of those).

@philippjfr

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

Okay thanks, sounds perfect! I'll be available for any amount of testing once you think it's ready for that.

@bryevdv

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

@philippjfr thank you for being available for testing, we will need all the help we can get. Also +1 to adding as much new downstream tests as possible.

@mattpap mattpap force-pushed the mattpap/8084_layout branch from e202d51 to 56c4a1e Sep 7, 2018

@xavArtley

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

I tried to run this code :

from bokeh.io import show
from bokeh.models import TickFormatter
from bokeh.plotting import figure
from bokeh.util.compiler import TypeScript

TS_CODE = """
import {TickFormatter} from "models/formatters/tick_formatter"
import {AxisView} from "models/axes/axis"

export class MyFormatter extends TickFormatter {
    type: "MyFormatter"

    // TickFormatters should implement this method, which accepts a lisst
    // of numbers(ticks) and returns a list of strings
    doFormat(ticks: string[] | number[], _axis_view: AxisView): string[] {
        // format the first tick as- is
        const formatted = [`${ticks[0]}`]
        for (let i = 1, len = ticks.length; i < len; i++) {
            formatted.push(`+${(Number(ticks[i]) - Number(ticks[0])).toPrecision(2)}`)
        }
        return formatted
    }
}
"""


class MyFormatter(TickFormatter):

    __implementation__ = TypeScript(TS_CODE)


p = figure()
p.circle([1, 2, 3, 4, 6], [5, 7, 3, 2, 4])

p.xaxis.formatter = MyFormatter()

show(p)

And I get this error:

    custom_impls = _compile_models(custom_models)
  File "c:\users\xavier\repos\bokeh\bokeh\util\compiler.py", line 615, in _compile_models
    raise CompilationError(compiled.error)
bokeh.util.compiler.CompilationError:
?[91merror?[0m?[90m TS6053: ?[0mFile 'c:/Users/Xavier/Repos/bokeh_custom_ext/Extensions/Ticker/extensions_example_ticking.py:MyFormatter.ts' not found.

If I put the TS_CODE in a separate file everything seems OK

@mattpap

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

Actually this isn't true anymore. In this PR tabs were promoted to a proper layout (under bokeh.models.layotus), though for compatibility you can still import them from widgets.

@mattpap

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

If I put the TS_CODE in a separate file everything seems OK

I guess this is a windows related issue, because otherwise we have examples with inline code that work (CI, locally on linux). Please start an issue for that.

@cutright

This comment has been minimized.

Copy link

commented Feb 1, 2019

I've created a new issue for the layout issues I described above #8614

xavArtley added a commit to xavArtley/bokeh that referenced this pull request Feb 2, 2019

xavArtley added a commit to xavArtley/bokeh that referenced this pull request Feb 11, 2019

xavArtley added a commit to xavArtley/bokeh that referenced this pull request Feb 19, 2019

mattpap added a commit that referenced this pull request Feb 21, 2019

mattpap added a commit that referenced this pull request Feb 21, 2019

Add a color picker and spinner widgets (#8678)
* Add a color picker and spinbox to widgets

* correct typescript indentation 4 spaces -> 2

* add callback attribute

* Improvement of color picker type handled

* rename spinner and warning about internet explorer in the docstring

* Correction of the HSL class documentation

* spinner in tests

* forgot to rename typescript

* Spinner improvements

* remove formatted strings

* trailing spaces

* Begin integration test implementation

* Add a new property which transform color to hexadecimal

* continue to add tests to spinner

* test correction

* py2 compat

* correct alphabetical order in __all__

* corrections post #8085

* code formatting

* Update and clean up widgets/{color_picker, spinner}

* Add a polyfill for Math.log10()

alhoo pushed a commit to alhoo/bokeh that referenced this pull request Feb 28, 2019

Add a color picker and spinner widgets (bokeh#8678)
* Add a color picker and spinbox to widgets

* correct typescript indentation 4 spaces -> 2

* add callback attribute

* Improvement of color picker type handled

* rename spinner and warning about internet explorer in the docstring

* Correction of the HSL class documentation

* spinner in tests

* forgot to rename typescript

* Spinner improvements

* remove formatted strings

* trailing spaces

* Begin integration test implementation

* Add a new property which transform color to hexadecimal

* continue to add tests to spinner

* test correction

* py2 compat

* correct alphabetical order in __all__

* corrections post bokeh#8085

* code formatting

* Update and clean up widgets/{color_picker, spinner}

* Add a polyfill for Math.log10()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.