-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Conversation
JamesHabben
commented
Nov 11, 2024
- 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
this is related to #3796 here is a sqlite db file that contains some sample data. the query from the previous screenshots is there is a pretty decent spread of content types in this db, including some null values. see the spread here: |
- 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
@apjarvis you might be interested in knowing about this pull request. It would be nice if you could review it as well. |
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 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. |
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.
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)
I think the risk is pretty low, but changing
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. |
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.
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 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. |
No more comments or reviews in this time, so I've just merged this. Thanks, @JamesHabben. |