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

JSON mode for cell editor #1173

Closed
wmertens opened this Issue Oct 15, 2017 · 61 comments

Comments

Projects
None yet
5 participants
@wmertens
Copy link

wmertens commented Oct 15, 2017

Details for the issue

It would be really handy if the cell formatter could handle JSON, much like for example http://jsoneditoronline.org/

Useful extra information

I'm opening this issue because:

  • DB4S is crashing
  • DB4S has a bug
  • DB4S needs a feature
  • DB4S has another problem

I'm using DB4S on:

  • Windows: ( version: ___ )
  • Linux: ( distro: ___ )
  • Mac OS: ( version: 10.13 )
  • Other: ___

I'm using DB4S version:

  • 3.10.1
  • 3.10.0
  • 3.9.1
  • Other: 3.10.99

I have also:

@pamtbaau

This comment has been minimized.

Copy link

pamtbaau commented Oct 17, 2017

Was just about to request the same enhancement. But simpler... beautifying the JSON text and minify on save, would already be a great improvement.

@mgrojo mgrojo self-assigned this Nov 15, 2017

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Nov 15, 2017

I'm working on this based on the QScintilla editor component we already use for the SQL tabs. Don't expect anything too fancy, just syntax-highlighting and folding.

Minifying is a different feature. For that the JSON extension could be used, but I think that should be left to the user. Maybe another developer can give his opinion about it.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Nov 15, 2017

Having a JSON mode "at all" would be a good start.

With the JSON1 extension, our Windows and OSX builds now all include it, as it's compiled into SQLite (and SQLCipher) itself.

For Linux, *BSD, and other unix like OS's... it would likely depend on whether the JSON1 extension is included with the provided SQLite. We can't assume it always would be, so we'd likely need to have the minifying be optional (if the extension is even present).

Does that help? 😄

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Nov 15, 2017

It helps, thank you. By the moment I will focus on just the JSON mode, it's enough for a challenge. 😄

@wmertens

This comment has been minimized.

Copy link
Author

wmertens commented Nov 16, 2017

mgrojo added a commit that referenced this issue Nov 18, 2017

JSON mode for cell editor
Support for JSON in the Database Cell editor using the QScintilla library.

The lexJSON has been added to the compilation.

See issue #1173

mgrojo added a commit that referenced this issue Nov 18, 2017

JSON mode for cell editor
Support for JSON in the Database Cell editor using the QScintilla library.

The lexJSON lexer has been added to the compilation, including the
necessary files from the QScintilla source package.

See issue #1173

mgrojo added a commit that referenced this issue Nov 18, 2017

JSON mode for cell editor
Support for JSON in the Database Cell editor using the QScintilla library.

The lexJSON lexer has been added to the compilation, including the
necessary files from the QScintilla source package.

See issue #1173
@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Nov 18, 2017

@wmertens and @pamtbaau the feature is implemented. Would you like to try it in the next nightly build?

@wmertens

This comment has been minimized.

Copy link
Author

wmertens commented Nov 18, 2017

@pamtbaau

This comment has been minimized.

Copy link

pamtbaau commented Nov 19, 2017

Version 3.10.99 (Nov 19 2017)
I can confirm that JSON mode is there and it does do syntax colouring...

Found the following issue: When quotes are misplaced/missing, all text turns white and turns red when windows loses focus.

json1

json2

mgrojo added a commit that referenced this issue Nov 19, 2017

Fix white over grey at invalid JSON in current line
The default style for invalid JSON or unclosed strings uses red
background and white foreground, but the current line has
precedence, so it is by default white over grey. We change the
default to something more readable for the current line at
invalid JSON. See issue #1173.
@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Nov 19, 2017

@pamtbaau, there was an incompatibility between our style for current line and the default style for invalid lines and unclosed strings. Now it should be solved.

mgrojo added a commit that referenced this issue Nov 22, 2017

Compact and indent on JSON cell editor
New setting for enabling the following feature:
when JSON data is loaded in the JSON cell editor, the text is
indented. Before JSON data written back into the cell, the text
is compacted.

See issue #1173

Minor adjustment in preference label: removed ":" for consistency.
@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Nov 22, 2017

@wmertens and @pamtbaau, now with minify, or in other words, the JSON data can be indented on loading, and compacted on storing. You have to activate the feature first in the preferences, Data Browser tab. Any feedback is welcome 😃

@pamtbaau

This comment has been minimized.

Copy link

pamtbaau commented Nov 23, 2017

A much better way to deal with JSON data this way! Thanks.

I see some room for improvement though... :

  • Unlike Text cell editor, JSON cell editor does not follow the font family set for the data browser.
  • When opening the data in JSON everything is highlighted. Not sure if that is desired.
  • When removing quotes and colons, syntax error is shown clearly. Not so when deleting (curly) brackets. An unmatched (curly) bracket only turns red when I place the mouse on it. This error is hardly noticable.
  • When switching from JSON to TEXT data remains indended. It should reverse to original.
  • Saving incorrect JSON into db should be prohibited, or a warning should be shown at least.
  • When importing incorrect JSON:
    • Missing quote: Text turns red.
    • Missing colon or bracket, no error indication.
    • A popup notifying the user would be appropriate when importing incorrect data.

Please don't shoot the messenger... 😇

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Nov 23, 2017

Sounds like real progress is being made. 😄

@wmertens

This comment has been minimized.

Copy link
Author

wmertens commented Nov 23, 2017

It's already a great improvement!

My notes:

  • IMHO, indent/compact can be the default. As long as you don't save, it doesn't make a difference anyway.
  • It would be great if text values are automatically checked for being parseable as JSON (if they are less than e.g. 100KB), and then the editor automatically switches to JSON.
  • If using the json() function, minification and verification is done automatically by sqlite:
    sqlite> create table t(j json);
    sqlite> insert into t values ("[5, 1,     2]");
    sqlite> insert into t values (json("[5, 1,     3]"));
    sqlite> select * from t;
    [5, 1,     2]
    [5,1,3]
    sqlite> insert into t values (json("[5, 1,     3"));
    Error: malformed JSON
    
  • Folding seems to have issues: folding an array with a nested array will fold to the close of the nested array:
    image
    image
@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Nov 23, 2017

Hi, @pamtbaau. Thanks for the feedback.

  • Unlike Text cell editor, JSON cell editor does not follow the font family set for the data browser.

The font set by default for the data browser is of variable width. I think the font for any kind of code must be monospace. That's why the SQL font is reused here and in the binary editor. The settings should be, maybe, reorganized or relabeled, but that's a bit revolutionary. Other opinions are welcome.

  • When opening the data in JSON everything is highlighted. Not sure if that is desired.

This is for consistency to the Text editor.

  • When removing quotes and colons, syntax error is shown clearly. Not so when deleting (curly) brackets. An unmatched (curly) bracket only turns red when I place the mouse on it. This error is hardly noticable.

That is how the Scintilla widget is working, no easy way to change it.

  • When switching from JSON to TEXT data remains indended. It should reverse to original.

I doubted about this, but probably it makes sense as you propose.

  • Saving incorrect JSON into db should be prohibited, or a warning should be shown at least.

A warning seems to me the best approach and will be easy to implement.

  • When importing incorrect JSON:
    • Missing quote: Text turns red.
    • Missing colon or bracket, no error indication.
    • A popup notifying the user would be appropriate when importing incorrect data.

A warning would make sense here as well.

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Nov 23, 2017

Hi, @wmertens, thanks for the feedback!

  • IMHO, indent/compact can be the default. As long as you don't save, it doesn't make a difference anyway.

I think that letting the user discover the preference is the safest approach, otherwise we can change his/her own format inadvertently.

  • It would be great if text values are automatically checked for being parseable as JSON (if they are less than e.g. 100KB), and then the editor automatically switches to JSON.

It would make sense, but currently the mode is not changing automatically, even when image and binary data is already detected. Could be considered in a global way if nobody sees a problem for that.

  • If using the json() function, minification and verification is done automatically by sqlite:

I've used the Qt JSON class because Qt is already a requirement. On the other hand, we cannot assume that a given extension is available.

  • Folding seems to have issues: folding an array with a nested array will fold to the close of the nested array:

Seems a Scintilla issue. I cannot reproduced your problem. Could you provide the full JSON data? I've found this patch for a folding issue in the JSON lexer, but seems to be another one. We have the version previous to the patch. https://groups.google.com/forum/#!topic/scintilla-interest/w4sW3sM6Cek

mgrojo added a commit that referenced this issue Nov 23, 2017

JSON validation before applying data to cell
A warning dialog is popped-up for confirming application of
invalid JSON data. Parse error is shown in the dialog.

*.json added to import filter.

See issue #1173
@wmertens

This comment has been minimized.

Copy link
Author

wmertens commented Nov 24, 2017

IMHO, indent/compact can be the default. As long as you don't save, it doesn't make a difference anyway.

I think that letting the user discover the preference is the safest approach, otherwise we can change his/her own format inadvertently.

Well, the setting is quite hidden, how about moving the setting to the preview pane? So then they can turn it on/off easily and it will be retained between sessions.

It would be great if text values are automatically checked for being parseable as JSON (if they are less than e.g. 100KB), and then the editor automatically switches to JSON.

It would make sense, but currently the mode is not changing automatically, even when image and binary data is already detected. Could be considered in a global way if nobody sees a problem for that.

Good point. For sure this belongs in a different issue; perhaps the type dropdown can instead be a button group and auto-detected formats will highlight the corresponding button, and there can be a "[x] autodetect format" checkbox

If using the json() function, minification and verification is done automatically by sqlite:

I've used the Qt JSON class because Qt is already a requirement. On the other hand, we cannot assume that a given extension is available.

Oh, I thought that the JSON extension is always compiled into this app now?

Folding seems to have issues: folding an array with a nested array will fold to the close of the nested array:

Seems a Scintilla issue. I cannot reproduced your problem. Could you provide the full JSON data?

I can even reproduce it without much nesting, as long as the json is longer than the view:

{
    "accessTime": "16:00:00",
    "address": {
        "country": "ES"
    },
    "agentId": "1234",
    "bedroomCount": 3,
    "calcCapacity": 6,
    "capacity": 6,
    "contentsOld": [
        {
            "count": 1,
            "itemId": "1"
        },
        {
            "count": 1,
            "itemId": "2"
        },
        {
            "count": 1,
            "itemId": "3"
        },
        {
            "count": 1,
            "itemId": "4"
        },
        {
            "count": 1,
            "itemId": "5"
        }
  ]
}

Actually, it is very odd. It seems to be related to auto-indent. When I first visit a cell, the first page will look ok but when I scroll down, the range indicators break up. If I then delete the json and undo, the whole json object has correct ranges.

mgrojo added a commit that referenced this issue Nov 24, 2017

Folding issues related to the Scintilla JSON lexer
Applied patch from:
https://sourceforge.net/p/scintilla/code/merge-requests/19/

See this thread for info:
https://groups.google.com/forum/#!topic/scintilla-interest/w4sW3sM6Cek

This is merged into Scintilla, so it might come fixed in a future
release.

This should solve the folding issue reported in #1173
@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Nov 24, 2017

@pamtbaau, the folding issue should be solved with my last commit. The JSON data is also validated before applying it back to the cell (previous commit).

@pamtbaau

This comment has been minimized.

Copy link

pamtbaau commented Nov 25, 2017

Feedback on Version 3.10.99 (Nov 25 2017)

  • Warning on apply works fine.
  • Warning on import not yet working.
@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Nov 25, 2017

I've only implemented the warning on apply. Warning on import seems redundant, doesnt't it? There is no import without later apply unless the user changes mind.

@pamtbaau

This comment has been minimized.

Copy link

pamtbaau commented Nov 25, 2017

There already is a clue that imported data is flawed because it doesn't expand. That behaviour might be confusing to the user. A warning would add a bit of user-friendliness...

I almost logged as feedback that import data did not get expanded automatically... to find out on further inspection that the data was malformed...

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Dec 11, 2017

What do you reckon, @justinclift ?

@mgrojo Let me try it out with some JSON, and I'll let you know. Might be tonight, but more likely tomorrow. Feeling a bit brain drained at the moment. 😉

mgrojo added a commit that referenced this issue Dec 11, 2017

Allow user to set the BLOB text. Preview for field display in Prefere…
…nces

A new setting allows the user to set the text for BLOB data in the cell.

The existing line-edit for setting the NULL text and the new one for BLOB
are updated with the cell background and foreground colors, providing a
preview of the desired changes. An additional preview-only box is added for
the regular fields.

See issue #1263

Settings in Database tab are left-aligned for consistency to other tabs.
See comment in #1173.
@wmertens

This comment has been minimized.

Copy link
Author

wmertens commented Dec 11, 2017

@mgrojo yey on all your replies, but the on->off on the button does not show the original in the latest nightly.

Of course, if you will be moving the pref back and enabling it by default, it doesn't matter :)

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Dec 11, 2017

I see. It's happening when you change from the Text mode to the JSON mode. If you have already set the JSON mode before entering the cell, it performs the formatting/compacting as expected. Let's see where the setting ends before trying to fix it.

mgrojo added a commit that referenced this issue Dec 15, 2017

Refactoring and find/replace dialog in the JSON editor.
SQL and JSON text editor classes have been refactored. A new parent class
for both editors have been added for the common logic implementable without
depending on the specific lexer.

The only visible effect of this change should be that the JSON editor
(issue #1173) now has the same find/replace dialog as the SQL editor.

This prepares for the implementation of the XML editor (issue #1253).
@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Dec 15, 2017

With the last commit the JSON editor has the same find/replace dialog as the SQL editor.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Dec 15, 2017

Oops, I didn't get around to trying this out. Sorry all. ☹️

@wmertens

This comment has been minimized.

Copy link
Author

wmertens commented Dec 21, 2017

@mgrojo I just noticed that the JSON formatting also sorts the keys. Can this be turned off? Key insertion order matters in JavaScript…

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Dec 21, 2017

@wmertens I hadn't noticed that. Apparently there is no way to tell the Qt libraries to preserve the original key order when formatting or creating JSON data. On the other hand, this is what the standard RFC7159 says, so I guess this is not uncommon:

JSON parsing libraries have been observed to differ as to whether or
not they make the ordering of object members visible to calling
software. Implementations whose behavior does not depend on member
ordering will be interoperable in the sense that they will not be
affected by these differences.

The only current way to avoid this, it's to totally disable formatting.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Dec 21, 2017

@mgrojo We should probably file a bug with the Qt project too, as it's reasonably common to come across cases where the ordering of JSON fields makes a difference. The Qt people might be able to suggest a workaround for keeping it unsorted too. Not guaranteed, but they sometimes suggest useful things in the bug comments. 😄

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Dec 21, 2017

@justinclift I've found this Qt bug report but unfortunately the feedback was very disappointing. Basically, 'you don't need that'. They even closed the issue and it is currently open only because the reporter insisted and reopened it.

@pamtbaau

This comment has been minimized.

Copy link

pamtbaau commented Dec 22, 2017

According to The JSON Data Interchange Syntax stardard (december 2017)

6 Objects
... and does not assign any significance to the ordering of name/value pairs

7 Arrays
... The JSON syntax does not define any specific meaning to the ordering of the values. However, the JSON array structure is often used in situations where there is some semantics to the ordering.

According to the Internet Engineering Task Force (IETF):

An object is an unordered collection of zero or more name/value pairs, where a name is a string and a value is a string, number, boolean, null, object, or array.

An array is an ordered sequence of zero or more values.

According to json.org:

JSON is built on two structures:

  • A collection of name/value pairs. In various languages, this is realized as an object, record, struct, dictionary, hash table, keyed list, or associative array.
  • An ordered list of values. In most languages, this is realized as an array, vector, list, or sequence.

An object is an unordered set of name/value pairs.

An array is an ordered collection of values.

Although the response to the bugreport may be a bit harsh, it is what it is... It's up to the program to preserve the semantics of 'order'.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Dec 22, 2017

On a side note, that Qt bug report is probably the clearest case I've yet seen of a Contributor License Agreement (CLA) creating roadblocks to project growth. 😦

Additionally, if the bug reporter had just included the sample code (it's only a few lines) directly in the bug report window, instead of including them as attachments... then the reviewer likely would have actually seen what he meant, and been able to comment constructively instead of blaming the CLA to avoid doing so. *sigh*

That's something to remember when filing potential future bug reports with Qt... always do stuff inline, never as attachments.

@wmertens

This comment has been minimized.

Copy link
Author

wmertens commented Jan 12, 2018

I wonder, would it be possible to use https://github.com/peteristhegreat/qt-json-editor instead? It keeps the order of the original JSON.

There is also https://codereview.stackexchange.com/questions/11849/qjsonview-a-qwidget-based-json-explorer-for-qt but that's for Qt4, and it seems to use QJson, and I don't know if that uses source order.

Seeing the JSON objects in the original source order is very nice, because it shows the first written items at the top.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Jan 12, 2018

https://github.com/peteristhegreat/qt-json-editor is probably a bad idea. It hasn't had any updates in 3 years, none of the 5 forks of it has any changes either.

https://github.com/dridk/QJsonmodel (Qt5), mentioned in the StackExchange post seems to get the occasional update though, but seems like it's a widget just for display purposes. Doesn't seem to include any editing capability.

And yeah, I personally agree that being able to display things in the same order as given would be preferable.

@wmertens

This comment has been minimized.

Copy link
Author

wmertens commented Jan 15, 2018

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Jan 28, 2018

@justinclift @MKleusberg Do you have an opinion about where should be the auto-format setting for the JSON and XML modes? I think it is a good idea to decide finally on this before the final release, so it is not changed later after been released in another way. If we decide for it to go in the cell editor, I'll try to fix the problem that @wmertens reported on 11 Dec 2017.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Jan 28, 2018

No opinion personally, as I haven't yet gotten around to trying these out. 😇

That being said, I'd tend to go with @wmertens's preference as he'll be using it directly, so so be in a good place to give real world feedback. 😄

@wmertens

This comment has been minimized.

Copy link
Author

wmertens commented Mar 20, 2018

I have been using this for a while now and while it's not perfect, it is already very useful.

It would be perfect if it:

  • formatting is only in the editor component, so switching modes doesn't change the text
  • preserves source order
  • wraps long string values to be multi-line

The first one might be attainable here, the second two are really up to the JSON editor component. Sadly I am not proficient in Qt; in the browser it would not take very long to make a component like this.

So unless you think there are improvements you can do, this is good for me to close…

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Mar 20, 2018

formatting is only in the editor component, so switching modes doesn't change the text

Do you mean switching editor modes or format mode?

preserves source order

We rely on the Qt library for this and it is not supported, so it would be cumbersome to implement.

wraps long string values to be multi-line

This is an option in the Scintilla editor library. We could make it a preference. It would apply to SQL, JSON and XML editors and it would wrap every line longer than the editor width (with visual indications in the border):
http://pyqt.sourceforge.net/Docs/QScintilla2/classQsciScintilla.html#ac04428d2f90c36458d68a673f107e40c
imagen

Do you mean that?

@wmertens

This comment has been minimized.

Copy link
Author

wmertens commented Mar 20, 2018

wrapping: Yes that would be great!!! I think that would require a toggle button in the editor though, sometimes you want to see keys more than values.

formatting: I don't have a use for formatted JSON data in my DB, but it's wonderful while editing. Converting the cell value to formatted-with-whitespace is a bit of a hack to make the editor work, and if there are no changes, then changing the editor type should not retain the temporary spaces. So for me, edit formatted but store compacted seems like a very sensible default and a toggle for that can be hidden in the preferences if so desired.

Come to think of it, the css editor in Chrome devtools is pretty nice. It looks like a text editor but it enforces strict formatting.

The tree editor on the right in http://jsoneditoronline.org/ is pretty much like that, except that adding a field is a bit fiddly.

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Mar 28, 2018

wrapping: Yes that would be great!!! I think that would require a toggle button in the editor though, sometimes you want to see keys more than values.

Sorry but I'm thinking on adding the setting to the Preferences dialog. It would apply to all the editors (SQL and cell editor) and everywhere else takes away useful space.

mgrojo added a commit that referenced this issue Mar 28, 2018

Setting for line wrap in Scintilla editors
A new setting is added to the 'SQL' tab of the Preferences dialog. It
enables the line wrapping in the editors with none/character/word/
whitespace boundaries.

See comments in issue #1173.
@karim

This comment has been minimized.

Copy link
Member

karim commented May 16, 2018

https://github.com/dridk/QJsonmodel (Qt5), mentioned in the StackExchange post seems to get the occasional update though, but seems like it's a widget just for display purposes. Doesn't seem to include any editing capability.

I don't know if this is helpful to the issue or not, but it is now editable.

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented May 16, 2018

We can explore that option. The development seems still stalled, but the implementation is in fact simple, being based on the Qt classes, so no major problem if we finally need to maintain it by ourselves.

I wouldn't replace the current text editor based on QScintilla, but add a new mode to the cell editor. I think there is advantages and disadvantages in each mode.

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Sep 28, 2018

I've opened #1546 for the enhancement request that was mentioned here, but I'm going to set this as closed for our next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment