GNOME Bugzilla – Bug 704685
EvDocument uri and info not set
Last modified: 2013-07-24 11:29:14 UTC
Created attachment 249809 [details] [review] tentative patch Using ev_document_get_uri and ev_document_get_title I noticed that (always null returned from the first and always segmentation fault calling the second) priv->uri and priv->info are not set when loading a document from a GFile through ev_document_load_gfile (while ev_document_load sets them correctly). Alessandro
Review of attachment 249809 [details] [review]: Good catch, the problem also happens when loading from a stream, in that case we can't set a uri, but we can definitely load the info. Thanks. ::: libdocument/ev-document.c @@ +418,3 @@ + priv = document->priv; + priv->uri = g_file_get_uri (file); + priv->info = _ev_document_get_info (document); I think we can move the info initialization to ev_document_setup_cache() so that it's also done for other load methods. I think we should also do the syntex initialization in this case, we could probably move it to a helper function and call it from here and ev_document_load.
OK moving priv->info initialization in ev_document_setup_cache. priv->synctex init should be done only when a uri is given someway and of course the document supports it, so we'll have a ev_document_setup_cache_for_uri (maybe with a better name!) called from ev_document_load and ev_document_load_gfile after ev_document_setup_cache to set priv->uri and priv->synctex_scanner when supported? Is this correct? Thanks
Created attachment 249887 [details] [review] code moved in helper functions
(In reply to comment #2) > OK moving priv->info initialization in ev_document_setup_cache. > priv->synctex init should be done only when a uri is given someway and of > course the document supports it, so we'll have a > ev_document_setup_cache_for_uri (maybe with a better name!) called from > ev_document_load and ev_document_load_gfile after ev_document_setup_cache to > set priv->uri and priv->synctex_scanner when supported? Is this correct? > > Thanks Not really, the syntex is not a cache but an initialization, so I would use a different method for that and call it in load and load_form_gfile. ev_document_initialize_syntex (document, uri); for example.
Review of attachment 249887 [details] [review]: Thanks. ::: libdocument/ev-document.c @@ +424,2 @@ ev_document_setup_cache (document); + ev_document_setup_cache_for_uri (document, g_file_get_uri (file)); g_file_get_uri returns a new allocated string, so you are leaking it here because ev_document_setup_cache_for_uri call g_strdup. As I said I think it's better to initializa the uri directly here and rename ev_document_setup_cache_for_uri to ev_document_initialize_syntex or something like that.
(In reply to comment #5) > Review of attachment 249887 [details] [review]: > g_file_get_uri returns a new allocated string, so you are leaking it here > because ev_document_setup_cache_for_uri call g_strdup. As I said I think it's > better to initializa the uri directly here and rename > ev_document_setup_cache_for_uri to ev_document_initialize_syntex or something > like that. You're right! thanks :) now I'm freeing it! I moved synctex initialization code in a new function. I'm attaching the patch
Created attachment 250015 [details] [review] now freeing uri when no more needed
Review of attachment 250015 [details] [review]: Fixed the issues mentioned below and pushed to git master, thanks! ::: libdocument/ev-document.c @@ +277,3 @@ + priv = document->priv; + + priv->uri = g_strdup (uri); I don't think we need a helper function for a single line, and in this case makes things more complicated, see below. @@ +289,3 @@ + filename = g_filename_from_uri (uri, NULL, NULL); + if (filename != NULL) { + priv->synctex_scanner = This doesn't build, priv hasn't been declared here. @@ +433,3 @@ + ev_document_setup_cache_for_uri (document, uri); + ev_document_initialize_synctex (document, uri); + g_free (uri); We don't need to duplicate the uri if we initialize it directly here with: priv->uri = g_file_get_uri (file);
(In reply to comment #8) > Review of attachment 250015 [details] [review]: > > Fixed the issues mentioned below and pushed to git master, thanks! > > ::: libdocument/ev-document.c > @@ +277,3 @@ > + priv = document->priv; > + > + priv->uri = g_strdup (uri); > > I don't think we need a helper function for a single line, and in this case > makes things more complicated, see below. > > @@ +289,3 @@ > + filename = g_filename_from_uri (uri, NULL, NULL); > + if (filename != NULL) { > + priv->synctex_scanner = > > This doesn't build, priv hasn't been declared here. I'm sorry! I feel so stupid :) I was getting mad in squashing and cherry-picking to organize everything in a single commit that I forgot to build it! :) Sorry and thanks for your patience. > > @@ +433,3 @@ > + ev_document_setup_cache_for_uri (document, uri); > + ev_document_initialize_synctex (document, uri); > + g_free (uri); > > We don't need to duplicate the uri if we initialize it directly here with: > > priv->uri = g_file_get_uri (file); Cool!