Skip to content
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

Update extension-formats.c #3799

Merged
merged 2 commits into from
Dec 13, 2024
Merged

Conversation

JamesHabben
Copy link
Contributor

  • resolved some warnings
  • added plist to json function
  • added deserialize plist to json function for some plists
  • added some safety checks to avoid crashes

- resolved some warnings
- added plist to json function
- added deserialize plist to json function for some plists
- added some safety checks to avoid crashes
@JamesHabben
Copy link
Contributor Author

previously, plist data was only being parsed to xml text. here is what that version looks like in the app.
image

the new plist to json function parses to unspaced and unindented json format to allow the built in json editor to display intuitively.
image

some plists are organized in a different way, identifiable by a $archiver:NSKeyedArchiver key value pair. the plist deserialized to json arranges the keys and values how they should be viewed by deserializing them.
image

@JamesHabben
Copy link
Contributor Author

JamesHabben commented Nov 11, 2024

this is related to #3796

here is a sqlite db file that contains some sample data.
SkypeSpacesDogfood-9188040d-6c67-4c5b-b112-36a304b66dad.sqlite.zip

the query from the previous screenshots is
select ZPROPERTIES, plist(ZPROPERTIES), plisttojson(ZPROPERTIES), plistdeserializedtojson(ZPROPERTIES) from ZMESSAGEPROPERTIES

there is a pretty decent spread of content types in this db, including some null values. see the spread here:
image

- update `load_formats_builtin` to work better with app
- update `plist`, `plistToJson` and `plistDeserializedToJson` to return original cell value if it doesnt validate as a plist
  - previously they returned a made up blob or null
@JamesHabben
Copy link
Contributor Author

The load_formats_builtin function is used to interact with the app when it is building the formats list for the dialog in ColumnDisplayFormatDialog.cpp and I have updated it as well. It queries the loaded extensions for functions that begin with load_formats_ and then queries each of those returns to get the format details. Once it has those, it dynamically builds the format list in the dialog. you can see a picture below.

image

Please let me know how you would like the cpp code contributed. I am holding it locally for now as I don't know if you want to commingle the extension with app code.

@mgrojo
Copy link
Member

mgrojo commented Nov 13, 2024

@apjarvis you might be interested in knowing about this pull request. It would be nice if you could review it as well.

@mgrojo
Copy link
Member

mgrojo commented Nov 13, 2024

Please let me know how you would like the cpp code contributed. I am holding it locally for now as I don't know if you want to commingle the extension with app code.

Interesting concept. It would be definitely useful. We have to think in detail the API that we will propose for third party extensions, since it would be great if sqlite extensions started to provide this information for us.

One thing we might want to include, even if we cannot exploit it yet, is having information about the inverse of each format function. That would provide the possibility to make display formats which are editable. Another thing is whether we want that function to include a db4s_ prefix so we don't get confused by chance if some extension already provides that name for other purposes. Or maybe we just call it, and if it doesn't have the correct output, we simply ignore it...

I don't have a preference on how to receive the changes in the application. You could use this PR, or we finish this one, and then we think about this topics in the next one updating the application (and the extension, if we change the API).

This approximately implements #1198, by the way.

@JamesHabben
Copy link
Contributor Author

Interesting concept. It would be definitely useful. We have to think in detail the API that we will propose for third party extensions, since it would be great if sqlite extensions started to provide this information for us.

Agreed. Along with your initial comment in #1198 (comment) as to this being in a DB4S extension or sqlite extension. I like the idea of these being in a sqlite extension format since it would allow their use in user written queries and not restrict their usage to the table browser. On the other hand, doing this much string work in c lang is a major pain.

One thing we might want to include, even if we cannot exploit it yet, is having information about the inverse of each format function. That would provide the possibility to make display formats which are editable.

For the same reason stated above, I would not want to rebuild the plist binary format from json in c lang. especially the NSArchiver serialization with the plist. The current deserialization discards some of the data that would be needed to serialize it.

Rather, I would propose that the API call for a read function and separately an edit function. If no edit function, the editor can somehow indicate the data is read only. As it stands, with one of these plist display formats applied, the editor lets the user apply updates. The applied update is written in the format displayed in the editor, so it will overwrite the binary data with text in this scenario. (as you mentioned in 2017, and is still happening)

Another thing is whether we want that function to include a db4s_ prefix so we don't get confused by chance if some extension already provides that name for other purposes. Or maybe we just call it, and if it doesn't have the correct output, we simply ignore it...

I think the risk is pretty low, but changing load_formats_builtin to db4s_load_formats_builtin is an easy enough change and no impact to any functionality inside or outside of db4s that I can think of. Then documentation can be created to inform any developers that would like to create other display formats that wouldn't be included in the db4s distribution. I don't think any documentation for that currently exists.

I don't have a preference on how to receive the changes in the application. You could use this PR, or we finish this one, and then we think about this topics in the next one updating the application (and the extension, if we change the API).
This approximately implements #1198, by the way.

I think it makes sense to move forward with this PR as is since it doesn't impact any core db4s code. The next PR will have a few cpp files updated to make this all work well. As you mentioned, we can update this extension again as the API is updated or solidified. Thanks for sharing that other issue. I agree that this is solving everything in there except the inverse write functionality.

@mgrojo
Copy link
Member

mgrojo commented Nov 14, 2024

I've taken a quick look at the changes and also tested it a bit on my platform, and it looks good to me, but I'll wait some time in case someone else wants to make a more thorough review of the changes to the extension.

Rather, I would propose that the API call for a read function and separately an edit function. If no edit function, the editor can somehow indicate the data is read only. As it stands, with one of these plist display formats applied, the editor lets the user apply updates. The applied update is written in the format displayed in the editor, so it will overwrite the binary data with text in this scenario. (as you mentioned in 2017, and is still happening)

That "rather" looks like we are not totally agreeing, but then what you write seems to be what I had in mind. That is, in the list of functions, there would be an optional inverse function. If that function is used as display format, the inverse function would be used to save the edited values back to the database. If the inverse field is empty, the display format would be read-only, as you expect (now it is only on the browser, not on the editor dock).

Moreover, I think we shouldn't use this concept only for display formats, the metadata associated to the SQLite extension should also include the necessary information for SqlUiLexer::setupAutoCompletion(), so we get syntax highlighting for those functions and autompletion.

The only contra that I see for this idea, compared to what I described in #1198 (comment) using a separate XML file, is whether the SQLite extension authors will be willing to add this "DB4S extension" on top of their SQLite extensions. If they do, we all will benefit, but maybe they are not users of DB4S and are reluctant to include this metadata in their source. On the other hand, doing it on our side would be a maintenance nightmare. But in general, I think it is a better approach, because the extension is still a single shared library file, and consulting the metadata is straightforward.

@mgrojo mgrojo merged commit c63484f into sqlitebrowser:master Dec 13, 2024
11 checks passed
@mgrojo
Copy link
Member

mgrojo commented Dec 13, 2024

No more comments or reviews in this time, so I've just merged this. Thanks, @JamesHabben.

@mgrojo mgrojo added sqlite-extension SQLite runtime loadable extensions extensions labels Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extensions sqlite-extension SQLite runtime loadable extensions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants