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

map instead of set in selection #12363

Closed
urbanogb opened this issue Jul 25, 2018 · 10 comments
Closed

map instead of set in selection #12363

urbanogb opened this issue Jul 25, 2018 · 10 comments
Labels
needs: clarification The issue does not contain enough information for the team to determine if it is a real bug

Comments

@urbanogb
Copy link

Bug, feature request, or proposal:

Proposal

What is the expected behavior?

In tables with pagination that is obtained by rest on databases, row objects are recharged and therefore elements in the set collection of the selection class remain orphaned, with map, since the rows have an id this would not happen.

What is the current behavior?

What are the steps to reproduce?

Providing a StackBlitz reproduction is the best way to share your issue.

StackBlitz starter: https://goo.gl/wwnhMV

What is the use-case or motivation for changing an existing behavior?

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

Is there anything else we should know?

@jelbourn jelbourn added the needs: clarification The issue does not contain enough information for the team to determine if it is a real bug label Jul 25, 2018
@jelbourn
Copy link
Member

@urbanogb it's not clear what you're referring to. Can you elaborate with links to specific code and examples?

@urbanogb
Copy link
Author

Ok

@urbanogb
Copy link
Author

urbanogb commented Jul 29, 2018

@jelbourn, a sample of propose for change in cdk selection is in

https://github.com/urbanogb/sample-sel/

@jelbourn
Copy link
Member

@urbanogb I'm still not clear which code in Angular Material you're referring to.

@urbanogb
Copy link
Author

The source is:
material2/src/cdk/collections/selection.ts

At the line #16, i propose change

private _selection = new Set();

by

private _selection = new Map<string , T>();

In the constructor add a new parameter: keyField this parameter is the name of the property of that is its unique key.

constructor(
private _multiple = false,
initiallySelectedValues?: T[],
private _emitChanges = true,
private _keyField = 'id') { ....

When data is obtained through a remote connection with a database, it is usually necessary to identify each row in order to later make changes to it.
It is also common to receive only one page of data, sorted by some criteria and filtered, all these options can be changed by the user, asking for another data by another order with another filter or simply another data page or perhaps a page of different size .
The SelectionModel class saves in one of its properties all the rows that have been selected, I propose to use Map instead of Set, the source code that I leave as an example, within the src / app / table directory, has a clumsy approximation of what would be what I propose.
There are things that are a bit difficult to address, such as "selecting all" the rows of a paged request, you may have to renounce that, or perhaps address it as a flag, but never saving each and every one of the objects in memory of the browser, that would be ineffective.
Perhaps a flag to select everything could help, and in this case it would be possible to save in the property _selection of the SelectionModel class, the unchecked elements, that flag would indicate if _selection refers to those marked or unmarked.

In summary, the problem that I want to solve with the proposal is that the data selected by the user is "lost" when the tables are fed by a request by http, since each time the data is received a new array is created and the _selection property of type Set point to an array that no longer exists.
With Map, the rows are identified by their key, that is independent of the times that the data is reloaded, their orders, filters etc.

I came across this problem and I deduced that the cause was the Set collection.

To prove it in my example, in
src / app / table / table.component.ts just change the line

import {SelectionModel} from './selection';

by

import {SelectionModel} from '@ angular / cdk / collections';

Once this is done, if rows are selected on page 1 and advanced to 2 when they return to 1, they are no longer selected.

Thank you very much for your patience, it is clear that I am not good at explaining things.

@jelbourn
Copy link
Member

I don't see how changing to a map would prevent entries from being orphaned. The user would still need to either clear the selection or mark specific items as not-selected in order to remove them from memory.

@urbanogb
Copy link
Author

I created the example with the material template: table, instead of importing the SelectionModel class from cdk / collectios, I have put an outline of the one I propose. you can try the example with my proposal and With the import of cdk / collections.

The example is not complete. the problem is not to delete the selection, since Map, like Set, has a clear method that allows you to erase everything. I have seen that unchecking everything in the original class goes from item to item, this is ineffective. Clear is adequate.

The problem is to select everything, in remote tables it is easy to have thousands or millions of records.

So remove the option to select / deselect everything, but I can make a proposal of that.

The idea is to add a property _containsSelected: boolean = true;

This property indicates whether the data in the _selected array is the selected or unselected items
 
Selecting everything would be to make a clear _selected and put that flag to false.

Inverting the selection would only change that flag.

A user would never mark many items with his mouse.

I must do this for a real case in my company, if you want, in a few days I send you the version of that class much more mature ir a New SelectionModelMap.

@urbanogb
Copy link
Author

urbanogb commented Jul 31, 2018

@jelbourn , I have created a new branch in the sample-sel App with the proposal Solution.

The brach name is markation

In this sample, selected row and marked row are diferent concepts.

Selected rows = marked rows

Or

Selected rows = All rows - marked rows,

With this sample The user selection persist on change page, Order, filter etc.

With material/Cdk/collections/selection.ts the user selection Is cleared on change page etc..

@urbanogb
Copy link
Author

urbanogb commented Aug 1, 2018

@jelbourn ,
"I don't see how changing to a map would prevent entries from being orphaned. The user would still need to either clear the selection or mark specific items as not-selected in order to remove them from memory."

Try this in console:
let x = new Set()
x.add({a:1})
x.has({a:1})
You obtain true?
NO, FALSE!!!!

In the table the rows are Objects, no primitive values

Set(Value) is the same of Map(Value,Value). Set is a Map with key=value

If a value is a primitive value (integer,string, etc.) key is the value, in other case key is a REFERENCE OF value.

@urbanogb urbanogb closed this as completed Aug 1, 2018
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: clarification The issue does not contain enough information for the team to determine if it is a real bug
Projects
None yet
Development

No branches or pull requests

2 participants