openpsa / jsgrid

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

datatype autodetection #46

Open flack opened 9 years ago

flack commented 9 years ago

Based on @bouks implementation and the feedback from @OlegKi, here's a proposal for a modified autodetection algorithm (written as pseudocode for better readability):

if (grid.datatype !== 'auto')
{
    grid.url = "" + grid.url //Cast URL to string
    //normal jqGrid setup as it was before
}
else
{
    if (grid.data !== null)
    {
        grid.datatype = 'local';
    }
    else if (grid.url !== null)
    {
        detected_datatype = do_ajax_request_and_detect_datatype_with_jquery();
        if (detected_datatype === null)
        {
            throw 'datatype could not be detected. Please specify it manually';
        }
        if (    grid.loadonce === true
            && (detected_datatype === 'xml' || detected_datatype === 'json'))
        {
            grid.datatype = 'local'
        }
        else
        {
            grid.datatype = detected_datatype;
        }
    }
    else
    {
        grid.datatype = 'clientSide';
    }
}

The autodetection should only run once, when the grid is initially constructed.

From the user's point of view, the behaviour would be like this:

Using local data:

$('#list').jqGrid({
    colModel: [ name: "colName"],
    data: some_data
});

using remote data:

$('#list').jqGrid({
    colModel: [ name: "colName"],
    url: "/backend.php"
});

using nothing at all:

$('#list').jqGrid({
    colModel: [ name: "colName"],
}).jqGrid('setGridParam', {data: some_data});

using remote data from the current URL:

$('#list').jqGrid({
    colModel: [ name: "colName"],
    url: ""
});
bouks commented 9 years ago

What happens if you change 'url' and reloadGrid ?

bouks commented 9 years ago

What i don't understand is that you mix :

and

These must be separate things.

flack commented 9 years ago

@bouks: If you change url, we could simply reset datatype to null. then, reloadGrid should work as expected

flack commented 9 years ago

I mix remote and local because both are valid settings for datatype. So if we want to detect datatype automatically, we must account for the loca case, or am I missing something?

bouks commented 9 years ago

"I think there is confusion in datatype since the early begining. "local" is not a datatype, it's a state. I think we must redefine 'local' out of datatype."

This is not semantic.

From :

https://github.com/openpsa/grid.js/issues/38#issuecomment-69717514

flack commented 9 years ago

Well, tracking state in a variable that the user change via config is problematic to say the least. So decoupling state tracking from config would be a good idea anyhow.

What i don't understand yet is how does your current implementation react with this code:

$('#list').jqGrid({
    colModel: [ name: "colName"],
    data: some_data
});

Does it understand that this is local data, or does it try to run an AJAX request?

Also, what about this one (Oleg mentioned it in some other discussion):

$('#list').jqGrid({
    colModel: [ name: "colName"],
    url: ""
});

in https://github.com/openpsa/grid.js/blob/master/js/grid.base.js#L2043 since you don't use strict comparison, the autodetection will not trigger, or am I reading it wrong?

bouks commented 9 years ago

For the first example and this moment, it understand that it is local data BUT there is no autodetection in local data (mean you must use datatype) because i implemented autodetection only for remote.

For the second, you're right, it would be better to change L2043 by

if(ts.p.url && ts.p.datatype != 'local'){
flack commented 9 years ago

But that change would break when datatype is function or clientSide. And in any case, "local" is a very common use case, so it would be good if the autodetection would work in this case

bouks commented 9 years ago

Note that "function" datatype was never managed in populate function.

If you have "clientSide" datatype then, logically, you don't evaluate "url" to true because you naturally don't fill url option.

bouks commented 9 years ago

I repeat, don't make confusion between data recognition and where data is stored.

flack commented 9 years ago

I'm trying to look at this from an end user perspective (i.e. without any knowledge of internals). data is a configuration setting, and the description reads like this:

An array that stores the local data passed to the grid. You can directly point to this variable in case you want to load an array data. It can replace the addRowData method which is slow on relatively big data

So, as an end user, I think to myself: When I pass data, why do I have to pass datatype as well?

flack commented 9 years ago

or more specifically, when I use this code:

$('#list').jqGrid({
    colModel: [ name: "colName"],
    data: some_data
});

why does jqGrid do an AJAX request against the current URL?

OlegKi commented 9 years ago

The discussion here seems be very long and can be continued...

What do you thing about the usage of auto-detection only in case if datatype: "auto" or datatype: ""? I mean that one can say: there is a new feature - auto-detection of the datatype. You can use datatype: "auto" or datatype: "" which will be new values allowed for `datatype parameter. All other values of datatype will work exactly as in old versions. Only if one explicitly want to use auto-detection (by specifying datatype: "auto" or datatype: "" as the option) the auto-detection will take place.

In the way one will have no side effects in existing code, but one can still change the value of datatype option to use the new feature, if one not sure about the format of input data.

What I mean is the changing of the logic to the following

var useAutodetection = ts.p.datatype !== "auto" && ts.p.datatype !== "";
$.ajax($.extend({
    // option without datatype
    success: function(data,st, xhr) {
        ...
        if (useAutodetection) {
            ...
        }
        ...
    }
},
useAutodetection ? {} : {dataType: ts.p.datatype },
$.jgrid.ajaxOptions, ts.p.ajaxGridOptions));

It will use dataType: ts.p.datatype option for the old code and will used new auto-detection logic (where no dataType are used in jQuery.ajax call) in case of usage datatype: "auto" or datatype: "" (independent whether url is "" or have another value).

I personally know exactly that my server returns JSON. So I would just continue explicitly specify datatype: "json".

bouks commented 9 years ago

@flack : because of the if(ts.p.url != null) that i propose to replace by if(ts.p.url)

@OlegKi : I wanted in the first to just autodetect in case of server side data. Now you want autodetect everywhere. :) In your example, you must specify the datatype to do ajax request, that is not autodetection and a way to simplification and automation.

For my algorithmic point of view, there is :

OlegKi commented 9 years ago

@flack I read your post after I posted my previous remark.

I full agree with you. It's difficult to understand for an end user (the developer which uses jqGrid) why one should specify datatype additionally. the problem is that the same data parameter could be filled internally by jqGrid too.

One will use the code like

$('#list').jqGrid({
    datatype: "json",
    colModel: [ name: "colName"],
    url: "someUrlWhichReturnAllData",
    loadonce: true
});

The code will first make Ajax request to url. The url could be some existing service which don't know any jqGrid parameters and it would just return all data as array of named items (with name of the column as property name of every item). jqGrid will fill HTML table only with the first rowNum rows of data and save all data in data parameter. After successfully processing of the loading and filling of the first page jqGrid will change datatype to "local". So one would have the grid with practically identical options like in case of local data. One don't need to implement any server code and the performance is really very good from the point of view of the end user who will work with the grid.

Only because of the implementation of loadonce: true behavior and because jqGrid have datatype: "xml" as default option the user who specify data option explicitly during creating the grid have to specify additional datatype: "local". I think that the usage of datatype: "local" would be the best choice as default value of datatype.

Look at the performance of the demo which have 90000 rows of local data and use the page size 20 rows. You can try to sort the data, and use paging. You will that modern web browsers like Chrome can process the number of data really quickly. Another demo loads 90000 rows and use 1000 rows in the page. Independent from the question whether one really need to display such long page one can see that the performance of grid is very good. The user can see the results more quickly as one have typical round trip for the request to the server.

What I mean is the usage of loadonce: true is recommended way if your server data are not really large (millions of rows). I should remark that one can still use server side editing of data because editurl is full independent from the url option.

I have now one more idea. One can remove datatype from the list of default option. It allows to test whether data parameter is specified during creating the grid. If it is specified and datatype is undefined one can safe set datatype: "local". If one don't find any data parameter then one can follow fallback scenario and to set datatype to the old value "xml". One can do the same with jsonReader or xmlReader options as additional (bbut still not 100%) criteria of detection which can force the usage of datatype: "json" instead of datatype: "xml" in some clear cases.

What do you think about the kind of detection?

OlegKi commented 9 years ago

@bouks : to be exact if(ts.p.url != null) means: if not null or undefined, but if(ts.p.url) means: if not null, "" (empty string), undefined, 0, false, NaN or even an object. It is not the same. So all depends on which condition you really want to test. The classical example

var b = new Boolean(false);
if (b) // this condition evaluates to true

By the way if b would be a function (if ts.p.url would be a function, which is allowed in many parts of jqGrid code) then if(ts.p.url) would be true.

So all depends on which condition you really want to test. I personally don't like if(ts.p.url) because if will be interpreted by JavaScript not in the same way like the most read would read the statement.

bouks commented 9 years ago

I practically always use loadonce: true.

I think

if(url && !local){
    doAjaxRequest();
    if(detection by mime type){
        add[JSON|XML]Data();
    } else {
        jgridInternalDetection();
    }    
} else if(data) {
    getLocalData();
    jgridInternalDetection();
}

Then no datatype anymore. Only 'url' or 'data' option. url mean server side data and data for local data.

bouks commented 9 years ago

Another advantage of removing datatype from "public properties" is that if the source format change (from a server flow that you can possibly have no influence on it), you don't have to change your (user) code.

bouks commented 9 years ago

In the same thought above, we can possibly imagine that the 'xmlReader", "jsonReader", "localRader" options could be (or not if default) in the data itself and not in options.

flack commented 9 years ago

@bouks Where does the local flag in your example come from? You said that datatype wouldn't be needed anymore, so I'm kind of unclear on that detail

bouks commented 9 years ago

The 'local' flag (i'll call it 'source') is a "private property" that jgrid has internal use. For exemple :

OR

bouks commented 9 years ago

Like i said, i'd like separated "where the data come from" and "what format is the data" that are from today mixed in a single property.

meh-uk commented 9 years ago

I think you guys need to come to a consensus. What's going to cause the least pain for end users?

bouks commented 9 years ago

I think the least pain for end users is to define the minimum options and let the software do the job.

bouks commented 9 years ago

i think by the minimum option, only the options THEY NEED, NOT the options the SOFTWARE "NEEDS". I think they (or we because we are also end users) need to set only where and only where is the data and NOT what it is.

flack commented 9 years ago

@OlegKi Yes, that is what i had in mind. The autodetection should only run once during the inital setup of the grid. I noticed that I forgot to write this in the original ticket, but I've updated the description now.

It should be possible to combine your idea with @bouks autodetection of remote data. i.e. if data is not set initially, we can send the request and then set json or xml as appropriate

bouks commented 9 years ago

I did autodetection for "local" data.

So what i see after working about populating grid is :

So i propose in first time :

In a second time :

meh-uk commented 9 years ago

Seems sensible

bouks commented 9 years ago

"seems" is a matter of point of view, it's not a fact. :)

The "first time" is really simple. The second can be managed.

bouks commented 9 years ago

To answer this :

https://github.com/openpsa/grid.js/issues/38#issuecomment-70565143

I started a datatype detection for both remote and "local". I'm waiting for a return of meh-uk about a test he did.

meh-uk commented 9 years ago

I might not have time until the weekend, and I might be wrong, so if other people are OK with it, I'm happy.

bouks commented 9 years ago

No problem Matthew. See you soon.

flack commented 9 years ago

Hi @bouks,

I just found the "hidden demo" that you recent comitted:

http://openpsa.github.io/grid.js/demos/remote.html

I fixed the ids for the click handlers, so the buttons now work as expected, but unfortunately, no request against the server is issued. I've debugged this a little and think it may have to do with your autodetection logic (or some bad interaction between it and the Oleg changes we've merged). I'm not so familiar with that code, could you take a look when you have some time?

flack commented 9 years ago

P.S.: The problem is here I think:

https://github.com/openpsa/grid.js/blob/master/js/grid.base.js#L2403

datatype is "local" when reloadGrid runs, and that is why it makes no request

flack commented 9 years ago

I've worked around the demo problem now by setting datatype explicitly. We can still find a more elegant solution, but in the meantime, we'll at least have a working example

bouks commented 9 years ago

Linked to :