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

Editing filtered DataTable cell impacts wrong row #8040

Closed
jburgy opened this issue Jun 29, 2018 · 6 comments
Closed

Editing filtered DataTable cell impacts wrong row #8040

jburgy opened this issue Jun 29, 2018 · 6 comments

Comments

@jburgy
Copy link
Contributor

@jburgy jburgy commented Jun 29, 2018

ALL software version info (bokeh, python, notebook, OS, browser, any other relevant packages)

bokeh 0.13.0, python 3.6.5, macOS High Sierra 10.13.4

Description of expected behavior and the observed behavior

Editing filtered DataTable cell impacts wrong row. Impacted cell is below targeted one when rows above it are filtered (DTINDEX_NAME/index_position confusion).

Complete, minimal, self-contained example code that reproduces the issue

from datetime import date
from random import randint

from bokeh.io import output_file, show
from bokeh.layouts import widgetbox
from bokeh.models import CDSView, ColumnDataSource, CustomJS, CustomJSFilter
from bokeh.models.widgets import DataTable, DateFormatter, NumberEditor, Slider, TableColumn

output_file("data_table.html")

threshold = Slider(title='Threshold', value=30, start=0, end=100, step=10)

source = ColumnDataSource(dict(
    dates=[date(2014, 3, i+1) for i in range(10)],
    downloads=[randint(0, 100) for i in range(10)],
))

filter_ = CustomJSFilter(args=dict(threshold=threshold), code='''
return source.data.downloads.map(x => x >= threshold.value)
''')

view = CDSView(source=source, filters=[filter_])

columns = [
    TableColumn(field="dates", title="Date", formatter=DateFormatter()),
    TableColumn(field="downloads", title="Downloads", editor=NumberEditor()),
]

table = DataTable(source=source, view=view, columns=columns, width=400, height=280, editable=True)

threshold.js_on_change('value', CustomJS(args=dict(view=view), code='''
view.compute_indices(view.source);
view.source.properties.data.change.emit()
'''))

show(widgetbox(threshold, table))

Increase value of a cell where index_position > current row, hit enter, and observe your new value appearing below the cell you edited.

Stack traceback and/or browser JavaScript console output

No error or warning in console.

@jburgy
Copy link
Contributor Author

@jburgy jburgy commented Jul 2, 2018

The issue arises when CellEditorView.applyValue passes item[DTINDEX_NAME] (index in the original source) as offset to DataProvider.setField. DataProvider.setField expects a view index and translates it to source index using this.index[offset]. Our choice is to

  • replace item[DTINDEX_NAME] by this.args.grid.getData().view.indices_map[item[DTINDEX_NAME]] in CellEditorView.applyValue or
  • replace this.index[offset] by offset in DataProvider.setField

What do you recommend?

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Jul 5, 2018

What do you recommend?

I can only recommend that you wait for the fix in an upcoming release, or downgrade to a version that is unaffected. There is a cluster of issues with DataTable / CDSView / Selection indices that are all inter-related (in fact I may close this issue as a dupe). I intend to address them all soon, including much better test coverage for DataTable overall, but the release is about two months away at best guess.

@jburgy
Copy link
Contributor Author

@jburgy jburgy commented Jul 7, 2018

As you probably noticed, we're heavy DataTable users over here and are happy to contribute if helpful. Feel free to share your plan and we can see if a piece can be logically carved out.

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 7, 2018

@jburgy apologies I did not see your generous offer of help earlier. It's simply getting very hard to keep up with the volume of GH, SO, mailing list, gitter, ... inputs coming my way. If you are still interested in submitting a PR with either of your suggested solutions, I would be happy to review/test it this week, including helping write an integration test using a newly updated selenium/integration test machinery.

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 8, 2018

@jburgy I did just now try your first suggestion above, but it did not seem to resolve the issue (unless I have misunderstood the change to be made which is quite possible). The second seems like it can't be workable out of hand, since it would seem to break sorting use cases. If you have a concrete diff that solves the issue for you I'd be keen to evaluate it.

@bryevdv bryevdv mentioned this issue Oct 9, 2018
6 tasks done
@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 9, 2018

Actually I think the first solution does solve the issue, not sure what was going on before. WIll be adding integration tests to the linked PR tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants