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

Copy, move and delete thumbnails #29

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

livanh
Copy link
Contributor

@livanh livanh commented Jun 11, 2017

When files are copied, moved or deleted, their thumbnails are re-generated from scratch (which can be CPU- or disk-intensive) and/or left behind without being useful anymore (taking up disk space).
This PR applies the copy/move/delete operations to thumbnails as well (if they exist), solving these problems.

Copy link
Member

@LStranger LStranger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall idea is good, although I would rather make a single function for those operations instead of duplicating code many times. Also I would rather allocate paths in the get_thumbnail_paths() function instead of using g_build_filename() everywhere.
Thank you very much for your work.

GFile* src_large = g_file_new_for_path(src_path_large);
GFile* dest_normal = g_file_new_for_path(dest_path_normal);
GFile* dest_large = g_file_new_for_path(dest_path_large);
g_file_copy (src_normal, dest_normal, G_FILE_COPY_OVERWRITE, NULL, NULL, NULL, NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this should be not copying operation but moving one instead. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it's fixed now


/* copy thumbnails, if existing */
if(ret == TRUE && g_file_is_native(src) && g_file_is_native(dest))
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider to honor "thumbnail_local" configuration variable in regards of decision if we have to create a copy for a file thumbnail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done

@@ -870,6 +904,39 @@ gboolean _fm_file_ops_job_move_run(FmFileOpsJob* job)

if(!_fm_file_ops_job_move_file(job, src, NULL, dest, path, sf, df))
ret = FALSE;

/* move thumbnails, if existing */
if(ret == TRUE && g_file_is_native(src) && g_file_is_native(dest))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about to delete thumbnail if destination file isn't native?
Also I would consider to honor "thumbnail_local" configuration variable in that regard.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for me. Done

@LStranger LStranger force-pushed the master branch 2 times, most recently from 13d44ef to bb91bc7 Compare April 28, 2018 19:16
@livanh
Copy link
Contributor Author

livanh commented Apr 13, 2019

I applied the suggestion, except for allocating paths in get_thumbnail_paths().
The reason is that I also modified load_thumbnail_thread() to use get_thumbnail_paths() (for consistency and avoiding repetition). In load_thumbnail_thread(), paths are allocated only once and reused many times – presumably to avoid multiple allocations – so I did something similar with my code.

@LStranger
Copy link
Member

This may create issues due to multithreading. Let me think about this optimization for release 1.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants