GNOME Bugzilla – Bug 741057
File properties should provide file size
Last modified: 2015-01-18 10:41:50 UTC
Displaying PDF file properties in evince does not show the file size of the PDF file (Adobe Acrobat Reader does). I think it's both useful and trivial to implement.
Which version is this about?
3.14.1 for example.
Created attachment 293669 [details] [review] Adds a "File Size" in the file properties window. It is missing from newest version. I'm not aware of it being present in previous versions. I have attached my proposed patch. It adds a "File Size" in the file properties window. It is show as the last property in the file property window. Also includes danish translation.
Created attachment 293674 [details] [review] File properties now includes file size.
Review of attachment 293674 [details] [review]: Thanks for the patch. See the comments below. Don't attach translations, those are done/committed separately by the translation team. ::: po/POTFILES.in @@ +67,3 @@ +# files added by intltool-prepare +data/evince-previewer.desktop.in +data/evince.desktop.in How is this related with the bug reported? ::: properties/ev-properties-view.c @@ +94,3 @@ { EvPropertiesView *properties = EV_PROPERTIES_VIEW (object); + Don't add spurious space replacement, these are not related with the issue. @@ +256,3 @@ +{ + // Get file size + FILE *fp = fopen(strstr(uri, "://")+3, "r"); // Strip URI to path Use Glib functions to obtain information from a file. Don't use c++ style for comments. Leave a space before parenthesis. See the rest of the code to get an idea (this apply to the rest of the patch).
Review of attachment 293674 [details] [review]: ::: properties/ev-properties-view.c @@ +73,3 @@ { SECURITY_PROPERTY, N_("Security:") }, + { PAPER_SIZE_PROPERTY, N_("Paper Size:") }, + { FILE_SIZE_PROPERTY, N_("File Size:") } Maybe we could use just Size: ? @@ +260,3 @@ + fseek(fp, 0L, SEEK_END); + unsigned long size = (unsigned long)ftell(fp); + fclose(fp); I'm not sure this is the best place for this, since it's blocking io in the main thread and run every time the dialog is shown. I think we could get this in the worker thread as part of the load job, together with the other info that is cached during load. Otherwise we could do it here but using async API. @@ +264,3 @@ + // Human readable file size + int i = 0; + const char* units[] = {"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"}; Use g_format_size() instead.
Review of attachment 293674 [details] [review]: ::: po/POTFILES.in @@ +67,3 @@ +# files added by intltool-prepare +data/evince-previewer.desktop.in +data/evince.desktop.in You're right, a mistake from my side. I've removed it in my updated patch. ::: properties/ev-properties-view.c @@ +73,3 @@ { SECURITY_PROPERTY, N_("Security:") }, + { PAPER_SIZE_PROPERTY, N_("Paper Size:") }, + { FILE_SIZE_PROPERTY, N_("File Size:") } I agree. Changed to "Size:". Fixed in new patch. @@ +94,3 @@ { EvPropertiesView *properties = EV_PROPERTIES_VIEW (object); + My bad, sorry. Fixed in new patch. @@ +256,3 @@ +{ + // Get file size + FILE *fp = fopen(strstr(uri, "://")+3, "r"); // Strip URI to path Fixed in new patch. @@ +260,3 @@ + fseek(fp, 0L, SEEK_END); + unsigned long size = (unsigned long)ftell(fp); + fclose(fp); I totally agree. I'm new to this codebase. It is now done in the load job. Fixed in new patch. @@ +264,3 @@ + // Human readable file size + int i = 0; + fseek(fp, 0L, SEEK_END); Fixed in new patch.
Created attachment 294235 [details] [review] Add file size "Size" in the file properties window. Hello, I really appreciate your review! I agree with all of your feedback. I've moved the stuff to be part of the document load job (like you suggested). Also using glib functions now.
Created attachment 294236 [details] [review] Add file size "Size" in the file properties window. Sorry, ignore my previous patch. I removed unwanted trailing whitespaces (there were some even in the existing code).
Review of attachment 294236 [details] [review]: This looks much better, I still have a few more suggestions. ::: libdocument/ev-document-info.h @@ +126,3 @@ double paper_width; EvDocumentLicense *license; + char *file_size; /* eg, 200KB */ I don't think this should be part of DocumentInfo struct, since this is used to get information that is specific to the backend, and the file size is common. ::: libdocument/ev-document.c @@ +24,3 @@ #include <stdlib.h> #include <string.h> +#include <glib.h> glib.h is already included in the header. @@ +296,3 @@ * * Loads @document from @uri. + * Still unrelated changes. @@ +332,3 @@ ev_document_setup_cache (document); document->priv->uri = g_strdup (uri); + document->priv->info->file_size = ev_document_file_size (document); Save the file_size in the EvDocumentPrivate struct. @@ +424,3 @@ ev_document_setup_cache (document); document->priv->uri = g_file_get_uri (file); + document->priv->info->file_size = ev_document_file_size (document); Ditto. @@ +437,3 @@ * * Saves @document to @uri. + * More unrelated changes @@ +522,2 @@ filename = synctex_scanner_get_name (scanner, synctex_node_tag (node)); + And here. @@ +748,3 @@ +gchar * +ev_document_file_size (EvDocument *document) So, you made this function public, but only used in this file. I think we could make like other public functions that return a cache value. We define _ev_document_get_file_size() as private function that calculates the size. This is the one called in the load job to save the file_size. And then we have a public ev_document_get_file_size() that returns a const gchar * and returns the already cached value. @@ +750,3 @@ +ev_document_file_size (EvDocument *document) +{ + FILE *fp = g_fopen (g_filename_from_uri (document->priv->uri, NULL, NULL), "r"); Since this requires that the uri has already been set before calling this method, I would add an assert to make sure we already have a uri. g_filename_from_uri() returns a new allocated string that you should free. Since this is called also when loading a GFile, we could have two private functions, _ev_document_get_file_size that receives the uri (instead of the document), and _ev_document_get_file_size_gfile that receives the GFile. The former simply creates a GFile and calls the latter. @@ +754,3 @@ + fseek (fp, 0L, SEEK_END); + unsigned long size = (unsigned long)ftell (fp); + fclose (fp); Use g_file_query_info with G_FILE_ATTRIBUTE_STANDARD_SIZE. ::: libdocument/ev-document.h @@ +182,3 @@ cairo_surface_t *ev_document_get_thumbnail_surface (EvDocument *document, EvRenderContext *rc); +gchar *ev_document_file_size (EvDocument *document); This should return a const gchar *. It should be get_file_size() (or even just get_size() since we might use this also to get the size when loaded from a stream in the future), for consistency with other getters. And the parameter name should be lined up with the others. ::: properties/ev-properties-view.c @@ +312,3 @@ return str; } + Lot of unrelated changes here. @@ +373,3 @@ g_free (text); } + set_property (properties, GTK_GRID (grid), FILE_SIZE_PROPERTY, info->file_size, &row); We need to change ev_properties_view_new to receive a document instead of a uri, and use ev_document_get_uri and ev_document_get_file_size.
Created attachment 294275 [details] [review] Add file size "Size" in the file properties window. I've done mostly what you did propose. I added a file_size, now together with the uri, in the EvPropertiesView. Maybe it should a pointer to the EvDocument?
Created attachment 294280 [details] Add file size "Size" in the file properties window.
Created attachment 294281 [details] [review] Add file size "Size" in the file properties window.
Created attachment 294282 [details] [review] Add file size "Size" in the file properties window.
Created attachment 294290 [details] [review] Add file size "Size" in the file properties window. hg-git messed up my patch. Fixed.
What about replacing the fseek() - ftell() combination with one fstat() call?
fseek is not used anymore my latest patch. Instead g_file_query_info and g_file_info_get_size are used.
Created attachment 294553 [details] [review] Add file size "Size" in the file properties window. Added missing g_strdup and g_uri_unescape_string.
Review of attachment 294553 [details] [review]: This looks much better! There are still some issues, and I have a few comments, but we are quite close. Thanks! ::: libdocument/ev-document.c @@ +24,3 @@ #include <stdlib.h> #include <string.h> +#include <glib/gstdio.h> Do you still need this now that you are using g_file_query_info? @@ +343,3 @@ ev_document_setup_cache (document); document->priv->uri = g_strdup (uri); + document->priv->file_size = _ev_document_get_file_size (uri, error); Do not pass this error to the function. That error is used to know if the document load has failed and we don't want to make the load fail just because we failed to get the file size. We could just return 0 when we fail to get the size. @@ +595,3 @@ + goffset size = 0; + + if (g_file_query_exists (file, NULL)) { This is very unlikely to happen, and g_file_query_info will fail anyway if the file doesn't exist. @@ +599,3 @@ + G_FILE_QUERY_INFO_NONE, NULL, error); + + size = g_file_info_get_size (info); g_file_query_info returns NULL in case of error (like if the file doesn't exist). So we can ignore the actual error (passing NULL) and return early here if we fail to get the size. @@ +612,3 @@ + GFile *file = g_file_new_for_uri (uri); + + gchar *size = _ev_document_get_file_size_gfile (file, error); Leave the variable declarations in a single block without empty lines. @@ +848,3 @@ +const gchar * +ev_document_get_file_size (EvDocument *document) It's a bit weird that something called get_file_size returns a string with the human readable representation of the size, I would call this get_file_size_for_display() for example. However, since the user can use g_format_size() I think it would be better to save the file size as a guint64 in the document, and this method will return that value. Also, since a document might be loaded using a stream, I think it would be better to call this ev_document_get_size() and we could save the stream size also when loaded with ev_document_load_stream() (we could do the stream part in a follow up patch). ::: properties/ev-properties-view.c @@ +379,3 @@ g_free (text); } + set_property (properties, GTK_GRID (grid), FILE_SIZE_PROPERTY, properties->file_size, &row); We could check if the size is > 0 do not show the property if it's 0. @@ +408,3 @@ NULL); + properties->uri = g_uri_unescape_string (ev_document_get_uri (document), NULL); + properties->file_size = g_strdup (ev_document_get_file_size (document)); Instead of saving the string here, we could save the guint64 and call g_format_size() when setting the property. That way we don't keep the string around all the time, and we can easily check if the size is valid or not (>0) before showing it.
Created attachment 294743 [details] [review] Add file size "Size" in the file properties window. Thank you for the great feedback and criticism! Here is a new patch containing improvements in relation to all your points.
Review of attachment 294743 [details] [review]: Perfect, I've just pushed it to git master with a few coding style modifications. ::: libdocument/ev-document.c @@ +40,3 @@ { gchar *uri; + guint64 file_size; /* eg, 200kB */ Now that we are keeping the uint64, the comment it not accurate, so I've removed it. The name of the variable is descriptive enough. @@ +586,3 @@ + GFileInfo *info = g_file_query_info (file, G_FILE_ATTRIBUTE_STANDARD_SIZE, + G_FILE_QUERY_INFO_NONE, NULL, NULL); + if(info) { if( -> if ( @@ +837,3 @@ +ev_document_get_size (EvDocument *document) +{ + g_return_val_if_fail (EV_IS_DOCUMENT (document), -1); We can't return -1 here, since the return value is unsigned. ::: properties/ev-properties-view.c @@ +374,3 @@ g_free (text); } + if(properties->file_size) { if( -> if (
Fantastic! Thank you!