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 for #254, How to have selected rows on table create with AsyncDataTableSource #272

Closed
wants to merge 4 commits into from

Conversation

MogensO
Copy link

@MogensO MogensO commented Mar 25, 2024

In _fixSelectedState we need to check for an entry in _selectionRowKeys for the SelectionState.none

In _fetchData we also need to ensure that _selectionRowKeys are maintained for the rows retreived from getRows

MogensO added 4 commits March 25, 2024 21:53
…ith AsyncDataTableSource

In _fixSelectedState we need to check for an entry in _selectionRowKeys for the SelectionState.none

In _fetchData we also need to ensure that  _selectionRowKeys  are maintained for the rows retreived from getRows
Fix for maxim-saplin#254, How to have selected rows on table create with AsyncDat…
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.94%. Comparing base (1919e3d) to head (af090e2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #272      +/-   ##
==========================================
+ Coverage   95.87%   95.94%   +0.06%     
==========================================
  Files           3        3              
  Lines        1140     1159      +19     
==========================================
+ Hits         1093     1112      +19     
  Misses         47       47              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maxim-saplin
Copy link
Owner

It doesn't seem to be working as expected. I.e. in async data source I set rows as selected:

image

Yet the state doesn't get applied:

image

@MogensO
Copy link
Author

MogensO commented Mar 27, 2024

This is weird.
If I do the same, I get:
image

I also tried to modify the Dessert domain model entity as shown below to simulate selected coming from the DataSource:
image
With the result being:
image

Could you eventually check your version of async_paginated_data_table_2.dart?
It should contain:
image

@maxim-saplin
Copy link
Owner

My bad, I had some older version of code, now I can see the selected state being applied... Yet there's still an issue.

When selected via UI, there's a change in subhead showing the number of selected row:
image

Yet when selection "programatically" via setting the field in the data row this doesn't get displayed:
image

There's a conceptual issue that needs to be fixed, the state of selection is stored internally by the widget and it also can be a part of data row itself. There's no proper sync of the states

@maxim-saplin
Copy link
Owner

I haven't given this enough though yet it might be a good point to keep the selection state out of data source (i.e. always ignore there data source selected), I don't know. Any suggestions?

@MogensO
Copy link
Author

MogensO commented Mar 27, 2024

Well not at the moment, I'll have to dig deeper in the code.
I'll give it a thought over the Eastern.
Anyway it is somehow a crucial function for me, as the selected state of a row
very often is something we will persists in a database and thus must be present in the data source.

@maxim-saplin
Copy link
Owner

maxim-saplin commented Mar 27, 2024

Conceptually the selection state looks like a transient one and shouldn't be part of entity persisted...

@MogensO
Copy link
Author

MogensO commented Mar 27, 2024

I don't really see why this should not be persisted.
If you i.e. select an order for picking in a WMS system you would certainly need to persist
that, i.e. because it signals that the order have been started and you or someone else would like
to see that later on somewhere else or maybe stop it again be deselecting it.
If that should be reflected somewhere else it would require a second column in the table which take up space
and essentially is redundant.
And on small handheld devices space is scarce.

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.

2 participants