Hey Martyn,
I have quite a bit on my plate lately with renovating houses and other
stuff, like finding a new customer etc, that I'll delay working on this
for some time for now.
If somebody wants to pick it up, I mostly agree with your review
commends and adapting the branch to fix these issues would be a great
start.
Perhaps I'll pick it up myself starting by fixing these issues. So if
somebody does start adapting the branch a little note would be
appreciated (to avoid doubling efforts).
Kind regards,
Philip
On Fri, 2013-01-04 at 15:20 +0000, Martyn Russell wrote:
> On 03/01/13 21:30, Philip Van Hoof wrote:
> > Hey Guys,
>
> Hello Philip,
>
> > Again, is somebody planning to review this? If not I'm planning to go
> > ahead and push it to master.
>
> I have just reviewed it briefly. My comments:
>
> 1. There are some white space issues here which need cleaning up.
> Trailing white space, etc. see
> src/libtracker-extract/tracker-extract-sparql.c.
>
> 2. Coding style too.
>
> 3. Shouldn't you use the tracker_sparql_* API properly in
> sparql_builder_finish() ? WE're appending strings here but we have APIs
> for inserting URNs and blank notes already which could make this a lot
> easier. We should use these:
>
> tracker_sparql_builder_object_blank_open() and _close()
> (unless you need to refer to _:file later on of course)
>
> and
>
> tracker_sparql_builder_object_iri()
>
> 3. Extra spaces before end of function are unnecessary ;)
> + g_clear_error (&error);
> +
>
> 4. I would have made sparql_builder_finish() take arguments in the order
> we use in TrackerExtractInfo for consistency, but that's me being pedantic.
>
> 5. tracker_extract_get_sparql() takes a "temp_file", is teh file
> temporary? Is it a URL or a path? we should be more specific with
> variables like this especially if this is new API.
>
> 6. No need for the check here, just strdup it... also, I would keep the
> variable name the same so it's clear what it is we're passing (i.e. a urn).
>
> + if (graph) {
> + data->graph_urn = g_strdup (graph);
> + }
>
> 7. Why not use -1 or a valid time_t for last_mod and last_access? Not
> sure why we need the extra _set booleans here.
>
> + if (last_mod != 0) {
> + data->last_mod = last_mod;
> + data->last_mod_set = TRUE;
> + } else {
> + data->last_mod_set = FALSE;
> + }
>
> 8. This looks a bit wrong to me. First, we always set the file from the
> temp_file variable given, but then we might set the url to be something
> different based on dest_url. Not only is this confusing but I expect it
> to cause problems inside tracker if the file used for the URL != file
> used for the PATH. I can see why you want to do this, but we didn't do
> it this way round with Nokia, we issued a separate update for the file
> details when it actually was saved as something else. The problem I see
> is that you could have a query which returns urn->path as /foo and
> urn->uri as file:///bar and that's clearly wrong:
>
> + data->storage = tracker_storage_new ();
> + data->file = g_file_new_for_path(temp_file);
> + if (dest_url) {
> + data->url = g_strdup (dest_url);
> + } else {
> + data->url = g_file_get_uri (data->file);
> + }
>
> You could ALWAYS set the url from the file and keep the dest_url as a
> separate struct member to help avoid any confusion and I think it would
> make a lot more sense here.
>
> 9. No need to use a variable here, you can just return the function
> value with a cast I imagine:
>
> + res = g_simple_async_result_get_op_res_gpointer (simple);
> +
> + return res;
>
>
> 10. Please put system includes before glib and libraries (coding style):
>
> +#include <glib.h>
> +#include <gio/gio.h>
> +#include <time.h>
> +
>
> 11. I wonder why you don't provide a sync API too?
>
> 12. This is wrong too:
>
> +#endif /* __LIBTRACKER_EXTRACT_ENCODING_H__ */
>
> 13. Copyright should probably be 2013 ;)
>
> 14. Why do we change src/libtracker-common/tracker-marshal.list at all?
>
> 15. I've not checked the whole on_fileinfo_received() part because it's
> really complex in the miner-fs already and we were fixing problems there
> for years related to parent existence and creation prior to insertion of
> a child path. This worries me about this branch because I get the
> impression from quick review that it's not really addressed fully. You
> also seem to allow parent URNs to not exist before adding a child to the
> database. That's not how we've done things in the miner-fs and this
> really breaks what people using the DB can expect. Correct me if I am wrong.
>
> 16. I presume the tracker-storage.[ch] addition to libtracker-extract is
> a straight copy of the files from libtracker-miner? I am not sure if
> this is such a good idea - but I've not been sure what to do here for a
> while now. From a quick git grep on the code, it seems we only use it in
> tracker-writeback, tracker-extract and tracker-miner-fs, and since
> miner-fs already links with libtracker-extract, it looks like the
> logical choice to move it there.
>
> 17. If you do plan to add tracker-storage.[ch] to libtracker-extract,
> you need to update the documentation so people know it wasn't added in
> 0.8 but in 0.16. ;)
>
> 18. White space issues in src/libtracker-miner/Makefile.am
>
> 19. I didn't see any patches for the documentation and since you're
> moving the whole tracker-storage.[ch] API to another library, you
> definitely need to do that.
>
> 20. For tracker-sparql, I would prefer a cleaner approach. Right now,
> it's really clear the utility is used for querying mainly. I think it
> would be better to use:
>
> "insert-file-path" instead of "metadata-file-path"
>
> i.e. insert instead of metadata for all new options there.
>
> It's then much clearer when looking at the options that they're about
> specifying data to insert. It's also shorter :)
>
> I also think that "metadata-file-path" is a bit long, can't we just use
> "insert-path"? The argument clearly states it's a FILE too. Same for
> "metadata-graph-urn", "insert-graph" is shorter and URN is the type it
> expects. Same again for "metadata-dest-url", would be better as
> "insert-url".
>
> Finally, I think that "metadata-available" needs to have a proper value
> type instead of NULL. It's not clear if "true" or "1" or "yes" are
> really the right thing to put in here, we should give an example of the
> boolean system being used here in the description text if it's still not
> clear.
>
> 21. White space issues in tracker-sparql.c
>
> 22. The reason I asked for a sync API is so we could use it in
> tracker-sparql ;)
>
> --
>
> NOTE: I have not compiled or tested this branch yet, this is just my
> review of the code.
>
> I can do some testing after some of the issues above are addressed - but
> I am a bit concerned to see it just committed in it's current state.
>
> Thanks for the work here all the same Philip!
>
--
Philip Van Hoof
Freelance software developer
Codeminded BVBA - http://codeminded.be
_______________________________________________
tracker-list mailing list
[email protected]
https://mail.gnome.org/mailman/listinfo/tracker-list
