openpsa / jsgrid

Fork of last jqGrid version before license change
http://openpsa.github.io/jsgrid/
Other
28 stars 12 forks source link

Toggle selection when clicking on a row in multiselect: false #2

Closed flack closed 9 years ago

flack commented 9 years ago

When you use single selection, you can highlight a row by clicking somewhere in it, but you can't remove the highlighting by clicking again (you can only move it to another row). This used to work around version 3.6 IIRC.

bouks commented 9 years ago

Is it a bug ? I mean, why would you like to remove highlighting (deselect) ?

flack commented 9 years ago

@bouks Not sure if it's really a bug or some usabilty quirk (out of the default labels, bug seemed the most approriate). But it alyways irritated me. For me, the highlight always implies that the row is "active" in some way. When you have multiple grids on the same page, you will end up with one row highlighted in each table eventually, and to me that always seems like having multiple mouse pointers :-)

OlegKi commented 9 years ago

I agree with bouks that it's not a bug, but I agree with flack that any behaviors where you can do something, but can't revert the step, are not nice. For example if you can scroll right, but scrolling left is not possible. The possibility to select without possibility of unselecting is not nice.

I write you before that I want to rewrite selection part of jqGrid. I implemented already an plugin for one customer with custom selection behavior of multiselect. During the implementation I found some bad implemented parts of selection or unneeded restriction.

For example one can't use multiselect with TreeGrid in jqGrid. It's pure implementation restriction. Another problem is the performance. If you would create a grid having many rows and without paging (or with large number of rows) then you will see how slow will be selection of all rows (click on the checkbox in the column header). One can change the implementation and to increase the performance dramatically in the case.

One more feature. Many people create the grid with multiselect:true option and hide the column with the checkboxes immediate ($("#grid").hideCol("cb");). So an interesting feature could be the grid with multiselect feature, but without the checkbox column at all. By the way I proved that the grid without the checkbox column makes selection about 3 times quickly. One sees the performance very clear on grids with large number of rows,

One more feature is "Windows like" multiselect where one could use Shift and Ctrl in combination with mouse click.

The next cool feature is holding of selection state over the pages. I mean if one select a row, go to the next page, go to the previous page and will see that the previously selected row stay selected. It seems to me a good feature and I plan to make it as default behavior, It should works with single rows selection and with multiselect.

All the features I have already implemented in the corresponding plugin. I need just to modify the code of jqGrid and to introduce some new options.

What is your opinion for the above features? Which new option you see @flack to implement unselestion behavior on click on previously selected row? Which other new features you would like to have from the area of selection?

Best regards Oleg

flack commented 9 years ago

@OlegKi I had already implementing the "unselect previously selected" behaviour once before (around the time of jqGrid 3.6 release). The change was even merged by Tony, but then it got lost during the 4.0 release. I had opened a ticket back then, but it was ignored. Unfortunately, I can't find my commit any more in the history of Tony's repo (and I don't have my old local copy any more), but it was really only a very small change. Basically the code had a check in the click handler that looked like

if (!multiselect && row_is_selected)
{
    return;
}

and the change was something like

if (!multiselect && row_is_selected)
{
    row_is_selected = false;
    return;
}

If it is up to me, I would not add a new option to control this behaviour (the configuration is complex enough as it is).

I like all you other ideas. The hiding of the checkboxes should be optional IMHO, and for keyboard-based selection I would add that on Mac that it would have to use the CMD instead of the CTRL key (because ctrl + click opens the browser's context menu), but that can be easily accomplished by using something like (event.ctrlKey || event.metaKey) IIRC

OlegKi commented 9 years ago

@flack Thank you for the feedback. I have no Mac (only iPads and iPhones) and don't know any Mac specifics. If would be very good if you could test some changes on Mac and to give feedbacks about the code changes to be more Mac complaint.

One wish to you: It would be very good to distinguish the implementation of some feature from the exact requirement or the exact specification of the behavior. For example I think that one can't just change jqGrid so that the second selection of the row would be interpreted as unselection in case of usage singe-select mode and if the row is previously selected. The existing code (inclusive jqGrid modules like form editing) can have multiple calls of setSelection method for the same rowid. On the other side it would be not so hard to change the code of the click handler so that the click on the previously selected row (in case of single-select mode) will unselect the row. Which lines of code would be changed is already another question. One more example is the callbacks which one calls in case of click on the row. There exist for example beforeSelectRow, onCellSelect, onSelectRow (onSelectRow will be called by setSelection) callbacks and jqGridBeforeSelectRow, jqGridCellSelect, jqGridSelectRow (called by setSelection) jQuery event. One can return false or "stop" string from beforeSelectRow/ jqGridBeforeSelectRow to prevent selection of the grid. I imagine to extend the possible values which can return beforeSelectRow/ jqGridBeforeSelectRow to inform jqGrid that unselect behavior of previously selected row is required. So it would be interesting to discuss with you, or everybody else who have the interest, the question which would be ideal behavior (the logic) of selection (single and multiselect) without going on the level of exact changes of the code. Which methods, it's options and parameters and the exact logic between need be changed?

bouks commented 9 years ago

Yes cool.

Sorry, i'm not with you at this moment due to what happen in my country. Hope i'll not miss important things.

Best.

Laurent

Le 10/01/2015 12:07, Dr. Oleg Kiriljuk a écrit :

I agree with bouks that it's not a bug, but I agree with flack that any behaviors where you can do something, but can't revert the step, are not nice. For example if you can scroll right, but scrolling left is not possible. The possibility to select without possibility of unselecting is not nice.

I write you before that I want to rewrite selection part of jqGrid. I implemented already an plugin for one customer with custom selection behavior of multiselect. During the implementation I found some bad implemented parts of selection or unneeded restriction.

For example one can't use multiselect with TreeGrid in jqGrid. It's pure implementation restriction. Another problem is the performance. If you would create a grid having many rows and /without/ paging (or with large number of rows) then you will see how slow will be selection of all rows (click on the checkbox in the column header). One can change the implementation and to increase the performance dramatically in the case.

One more feature. Many people create the grid with |multiselect:true| option and hide the column with the checkboxes immediate (|$("#grid").hideCol("cb");|). So an interesting feature could be the grid with multiselect feature, but without the checkbox column at all. By the way I proved that the grid without the checkbox column makes selection about 3 times quickly. One sees the performance very clear on grids with large number of rows,

One more feature is "Windows like" multiselect where one could use Shift and Ctrl in combination with mouse click.

The next cool feature is holding of selection state over the pages. I mean if one select a row, go to the next page, go to the previous page and will see that the previously selected row stay selected. It seems to me a good feature and I plan to make it as default behavior, It should works with single rows selection and with multiselect.

All the features I have already implemented in the corresponding plugin. I need just to modify the code of jqGrid and to introduce some new options.

What is your opinion for the above features? Which new option you see @flack https://github.com/flack to implement unselestion behavior on click on previously selected row? Which other new features you would like to have from the area of selection?

Best regards Oleg

— Reply to this email directly or view it on GitHub https://github.com/openpsa/grid.js/issues/2#issuecomment-69452144.

flack commented 9 years ago

@OlegKi sorry, I think my ticket description was a bit vague. I am only talking about direct user interaction, i.e. when a user clicks on a row he has previously already clicked on. I was not talking about API methods. If you call setSelection with a row that is already selected, it should stay selected IMO. Otherwise, the function would need to be renamed to toggleSelection. In fact, what I'm proposing is to make a mouse click on a row toggle its selected status (the same way it already does in multiselect)

OlegKi commented 9 years ago

I remind me that one old version of jqGrid had the feature, but it was reverted in the next version because of some compatibility problems with existing code. Probably it was because one implemented the changes inside of setSelection instead of inside of click handle. The implementation of the feature is very simple. Probably one can change the behavior of click to what you suggested, but to have small new option which allow to prevent deselection on click. So if somebody really will have problems, one can use the option to have the old behavior. What do you think @flack about such compromise? Which name would be good for such new option?

flack commented 9 years ago

@OlegKi if it were up to me, I would just implement the change and add an option if some user complains about the new behaviour. jqGrid has way too many options already IMO. But if you want to add a new option, I would probably make it look like this:

singleSelectClickMode: ['toggle', 'selectonly']

where toggle is the default setting (which behaves as described above) and selectonly is the old jqGrid behaviour.

flack commented 9 years ago

This was implemented by Oleg here: https://github.com/OlegKi/jqGrid/commit/702e18f1dda0f305addf20e9d3a72ed6131d3436

@bouks @meh-uk Any objecttions to merging this change? It's only six lines, so the chances of breakage should be minimal

bouks commented 9 years ago

What about if you click a row (then select it) and then, click on an inline action button ? Is this deselect it ?

flack commented 9 years ago

Just tried Oleg's example here:

http://www.ok-soft-gmbh.com/jqGrid/OK/navButtons2-fa4.htm

Selecting and then clicking the edit button in fact deselects. This is not a good behavior I think, so we should correct that after merging. I would say that the click event from the inline buttons should not bubble, but any click on an inline button should select the row if it is not already selected. Does that sound reasonable to you?

bouks commented 9 years ago

Also if you are editing the line, it should be impossible to deselect it.

flack commented 9 years ago

I just checked, this behaviour already exists when multiselect is true, for example, try editing here:

http://openpsa.github.io/jsgrid/demos/kitchensink.html

so we should probably open a separate ticket for that

flack commented 9 years ago

Implementation is merged now