free-jqgrid / jqGrid

jQuery grid plugin
https://github.com/free-jqgrid/jqGrid
Other
480 stars 196 forks source link

Random row ID when key is defined in colModel + drag&drop issue #427

Closed alexpicolin closed 6 years ago

alexpicolin commented 6 years ago

Hi all,

using jqGrid for years, it was time to upgrade the old version 4.6.0 to most recent (free) version 4.15.3. Unfortunately, applying it to my existing grids, two issues appeared.

(1) I am using XML to generate data for the grid that generates just standard stuff like

<row id="GBR">
    <cell>GBR</cell>
    <cell>UK</cell>
    <cell>Great Britain</cell>
    <cell>London</cell>
    <cell>Europe</cell>
</row>

By default, it generates GBR as id for the grid table row. However, when I define the key: true property in colModel for the second column (just as example), it does not replace GBR with UK, but generates a random ID like jqg123. It does not help when I remove the [id] attribute from <row>. I checked the source file jquery.jqgrid.src.js, and might have found out the reason. From line 4043 onwards, the scripts tries to find out idName and idIndex. From line 4139 onwards, it uses the information to create the id for the table row. If not found, the random ID generator is used. However, because I am using XML, it overwrites idName with a node reader that fails because my nodes use the standard <cell> syntax, and not a named syntax. I have applied a local fix like following:

4048    if (arrayReaderInfos[idName]) {
4049        idIndex = arrayReaderInfos[idName].order;
4050    }
4051    ELSE if (isXML) {
            [...]
4057    }

Even if XML, the arrayReaderInfos returns proper information from which the order can be read, meaning the idName is valid and does not require a node reader or similar. Therefore, I added the else to use the attribute/node readers only if array is unreadable. Did I miss anything in the settings or misunderstood the meaning of these lines?

(2) The reason why I need to set the key is for the drag&drop feature between two tables. Otherwise, when I move a row from table A to table B, the table row ID from the [id] attribute is lost and replaced again with a random ID generator. Do I miss any other feature here? However, the other issue is, when table A and table B have both rownumbers: true and multiselect: true, meaning two additional columns are created on the left, the column data of the dropped row are shifted to the left. This is because, a counter i is incorrectly initialised to zero in line 17456. This is because getdata (line 17454) only contains the 5 data columns (taking the example above) but dropmodel (line 17457) has not only the 5 data columns but also the rownumber and multiselect columns. I have to apply following lines to ensure that the both arrays match each other in the loop.

i+=$("#"+$.jgrid.jqID(this.id)).jqGrid("getGridParam","rownumbers")===true?1:0;
i+=$("#"+$.jgrid.jqID(this.id)).jqGrid("getGridParam","multiselect")===true?1:0;
i+=$("#"+$.jgrid.jqID(this.id)).jqGrid("getGridParam","subGrid")===true?1:0;

Do you mind checking whether I am wrong with any of the both issues reported, or if you can fix the issues in your scripts so that I don't need to embed a local copy with personal fixes?

OlegKi commented 6 years ago

Hi Alex,

could you provide the demo (jsfiddle demo, for example), which reproduce the problem? It'd difficult to examine the problem without be able to reproduce it.

About the problem with generating of rowids by gridDnD I'd recommend you to use callback function autoid (see the lines of the code)

alexpicolin commented 6 years ago

Hi Oleg, thanks for quick response. I created two jsfiddle demos for each issue reported.

(1) https://jsfiddle.net/1ybhz15d/39/ The second grid reflects the problem. Interestingly, when you trigger a reload of the second grid with help of the button, and click again a row, now the key is correctly generated. Why not from the beginning? (2) https://jsfiddle.net/zocdqtj7/13/ Drag and drop the rows and see what I've tried to explain (in a complicated manner, I must admit).

If I missed any configuration in the grids that causes any of these unexpected behaviors, please let me now.

Regarding your recommendation, I know the option autoid. If true, it generates something like dnd_1, if false, it generates jqg1 unless (!) one of the columns are generated with key: true. That's a strange behavior. It would rather assume that the original row id (from <tr role="row" id="GBR" ...>) remains and is not overwritten by default. Using a callback only allows me to access the getdata information which has all columns but not the originally generated row id. Anyway, if issue (1) is solved permanently, I can use ´key: trueand don't bother with theautoid` property.

OlegKi commented 6 years ago

Thanks for the demos. First of all about drag&drop between tables. I wrote you in my previous comment about the usage of autoid as callback function not as Boolean. The modified demo will be like https://jsfiddle.net/zocdqtj7/20/, where I used the following options of gridDnD

dropbyname: true,
autoid: function (item) {
    return item.code;
}

I'll analyse the first demo later too and will post you additional comment.

alexpicolin commented 6 years ago

Thanks for the update. The dropbyname property solves issue (2) with the shifting columns.

In terms of the ID for the drag&drop, I understood that autoid can be used as callback function, but in your code, the item object only contains the both columns (e.g. item.code and item.name) but I have no access to the id of the row. In your updated fiddle, the first row has GBR as id, UK as code and Great Britain as name. After drag&drop to the other table, with return item.code, you place UK as id.

Original state: <tr role="row" id="GBR" ...><td role="grid-cell">UK</td><td role="grid-cell">Great Britain</td>

After drag&drop, same row in second table (after addRowData is called) <tr role="row" id="UK" ...><td role="grid-cell">UK</td><td role="grid-cell">Great Britain</td>

I would assume that GBR remains if I keep autoid untouched or set it to false. Anyway, I can work around by adding a hidden column that has GBR in it and flag that as the key column - if issue (1) is solved.

OlegKi commented 6 years ago

Sorry, I'm very busy now with my main job, which has almost no relation to jqGrid. Thus, I post you now only the short answer. One can use beforedrop to extend/replace any data used in

dropbyname: true,
beforedrop: function (ev, ui, item, $srcGrid) {
    // extend the data inserted to the grid with additional information
    item.id = $(ui.draggable).attr("id");
    return item;
},
autoid: function (item) {
    return item.id; // uses the information added by beforedrop
}

See https://jsfiddle.net/zocdqtj7/32/. I hope it should be close to your requirements. I think that I will change the code of gridDnD to fix some bugs and to add more information to autoid callback.

OlegKi commented 6 years ago

I committed the fix to GitHub. Now your original code works if one uses the latest version of jquery.jqgrid.src.js or jquery.jqgrid.min.js from GitHub. See https://jsfiddle.net/1ybhz15d/50/. Additionally I extended autoid with additional options parameter, which properties contains additional helpful information inclusive rowid property (see the commit). Thus the code from my previous comment could be changed to the following:

var gridDnDOptions = {
        dropbyname: true,
        autoid: function (item, options) {
            return options.rowid;
        }
    };

jQuery("#jqGrid-w1").jqGrid("gridDnD",
        $.extend({ connectWith: "#jqGrid-w2" }, gridDnDOptions));
jQuery("#jqGrid-w2").jqGrid("gridDnD",
        $.extend({ connectWith: "#jqGrid-w1" }, gridDnDOptions));

See https://jsfiddle.net/zocdqtj7/35/, which uses the latest code of free jqGrid from the GitHub.

alexpicolin commented 6 years ago

Dear Oleg, thanks for your time spent on the updates and your feedback. I just installed the newest commit to my project. The IDs are now correctly set based on the key: true settings. And the gridDnD works perfectly with help of dropbyname: true and the new feature to read the rowid in the autoid callback. That's really appreciated and more than welcome. Now I can get rid of my local fixes.