Skip to content
This repository has been archived by the owner on Jan 1, 2020. It is now read-only.

datatype autodetection #46

Open
flack opened this issue Jan 13, 2015 · 36 comments
Open

datatype autodetection #46

flack opened this issue Jan 13, 2015 · 36 comments
Milestone

Comments

@flack
Copy link
Contributor

flack commented Jan 13, 2015

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
Copy link
Contributor

bouks commented Jan 13, 2015

What happens if you change 'url' and reloadGrid ?

@bouks
Copy link
Contributor

bouks commented Jan 13, 2015

What i don't understand is that you mix :

  • is the data from local or remote (that is already managed by grid in case of loadonce)

and

  • automatic recognition

These must be separate things.

@flack
Copy link
Contributor Author

flack commented Jan 13, 2015

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

@flack
Copy link
Contributor Author

flack commented Jan 13, 2015

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
Copy link
Contributor

bouks commented Jan 13, 2015

"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 :

#38 (comment)

@flack
Copy link
Contributor Author

flack commented Jan 13, 2015

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
Copy link
Contributor

bouks commented Jan 13, 2015

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
Copy link
Contributor Author

flack commented Jan 13, 2015

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
Copy link
Contributor

bouks commented Jan 13, 2015

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
Copy link
Contributor

bouks commented Jan 13, 2015

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

@flack
Copy link
Contributor Author

flack commented Jan 13, 2015

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
Copy link
Contributor Author

flack commented Jan 13, 2015

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
Copy link
Contributor

OlegKi commented Jan 13, 2015

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 ``datatypeparameter. All other values ofdatatype` 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
Copy link
Contributor

bouks commented Jan 13, 2015

@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 :

  • First, set data location ('local' or 'url')
  • Second, get data and autodetect format according what is received

@OlegKi
Copy link
Contributor

OlegKi commented Jan 13, 2015

@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
Copy link
Contributor

OlegKi commented Jan 13, 2015

@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
Copy link
Contributor

bouks commented Jan 13, 2015

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
Copy link
Contributor

bouks commented Jan 13, 2015

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
Copy link
Contributor

bouks commented Jan 13, 2015

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
Copy link
Contributor Author

flack commented Jan 13, 2015

@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
Copy link
Contributor

bouks commented Jan 13, 2015

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

  • I set 'url', then jgrid set source = 'remote'
  • jgrid get the data from the url ant then, if loadonce = true, jgrid set source = 'local'

OR

  • I set 'data', then jgrid set 'source' = 'local'

@bouks
Copy link
Contributor

bouks commented Jan 13, 2015

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
Copy link
Contributor

meh-uk commented Jan 13, 2015

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

@bouks
Copy link
Contributor

bouks commented Jan 13, 2015

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

@bouks
Copy link
Contributor

bouks commented Jan 13, 2015

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.

@meh-uk meh-uk added this to the 1.0 milestone Jan 14, 2015
@flack
Copy link
Contributor Author

flack commented Jan 14, 2015

@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
Copy link
Contributor

bouks commented Jan 15, 2015

I did autodetection for "local" data.

So what i see after working about populating grid is :

  • There is much confusing in terms and option definitions. I repeat the "from" and the "what" are mixed, that shouldn't be.
  • There is too much datatypes. For example, the difference between "json" and "jsonstring" is only from where it comes from. "json" from remote, "jsonstring" from local. They are the same except only a very little cleaning difference in jsonstring.
  • Idem with "xml" and "xmlstring"
  • "clientside" can be removed, it is just an alias to "local".
  • "javascript" is not managed, only "script" exists that is an alias to remotes data (json, xml...) and the ajax call.
  • "function" is not managed.

So i propose in first time :

  • remove datatype from "public" options in the first. This can be done without breaking compatibility. Users can remove the option, but if not, no problem.

In a second time :

  • remove "url" and "datastr" options and keep only "data" option that maybe eventually renamed "src". We can eventually do aliases for compatibility and remove them in the future or just do an alert to the user if he uses the wrong option.
  • we, then, can detect if the "src" (or "data") is a url or not.
  • add "local" internal option to manage loadonce and already loaded in-page data.

@meh-uk
Copy link
Contributor

meh-uk commented Jan 15, 2015

Seems sensible

@bouks
Copy link
Contributor

bouks commented Jan 15, 2015

"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
Copy link
Contributor

bouks commented Jan 19, 2015

To answer this :

#38 (comment)

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
Copy link
Contributor

meh-uk commented Jan 19, 2015

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
Copy link
Contributor

bouks commented Jan 19, 2015

No problem Matthew. See you soon.

@flack
Copy link
Contributor Author

flack commented Jan 22, 2015

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
Copy link
Contributor Author

flack commented Jan 22, 2015

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

This was referenced Jan 31, 2015
flack added a commit that referenced this issue Feb 6, 2015
@flack
Copy link
Contributor Author

flack commented Feb 6, 2015

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
Copy link
Contributor

bouks commented Feb 7, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants