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 files to journal #469

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ruhugu
Copy link

@ruhugu ruhugu commented Jan 19, 2020

Added support for copying files into the journal directory.

This closes #163.

I have added the method add_file() to the Journal class, following this comment. This method copies the given file into the directory media/-/ inside the journal directory, creating the required folders.
I have also changed the open_journal() method to create the media directory if it does not exist (I am not sure if this is the best place to do this).

In order to avoid overwriting when copying two different files with the same name, the previous method uses the function safecopy(), which has been added to the filesystem module. This function adds a counter to the destination filename if different file with the same name is found in the destination folder. The new names are generated with the function _safecopy_filename_generator().

In order to copy the files through the GUI, I have modified the on_insert_pic() and on_insert_file() methods to have an option to copy the files in the file_chooser dialog. I have set copy as the default behaviour for pictures but not for the files.

Finally, I have changed the select_journal() method in gui/menu.py to copy the journal media directory if "Save As" is used. To do so, I have added a new function copytree() in util/filesystem to copy recursively and change permissions so that only the user can read and write.

Even though the application works as expected with the previous changes, there are some additional ones that are desirable but I have been unable to do:

  1. Add the folder name "media" to Filenames.dirs. Right now, the folder name "media" is hardcoded in several places in the code. I think it would be nice to add it to dirs, so that both the folder name "media" and its full path can be retrieved from a method or attribute. However, I do not understand the structure of Filesname.dirs.
  2. Show an error dialog when copying a file fails. This should be easy, but I am clueless about gtk.
  3. Add a function to remove unused files in media directory. This would involve checking which files are not refered in the journal and remove them.

@jendrikseipp
Copy link
Owner

Thanks for the patch and the detailed description! I agree that points 1 and 2 would be good to have, but I think point 3 doesn't warrant the extra code complexity. We can always add it later if I'm wrong :-)

Points 1 and 2 also don't necessarily have to be part of this PR.

Regarding name clashes, I think it's better to let users choose the destination filename (with suitable defaults) and show a warning if the name already exists.

Regarding defaults, I thought it would be best to use the same defaults for everything, but now that I think about it, I like your solution better.

Copy link
Owner

@jendrikseipp jendrikseipp left a comment

Choose a reason for hiding this comment

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

I had a quick look at the code.

logging.error(
'Creating journal media directory failed: {}'.format(err))
return

Copy link
Owner

Choose a reason for hiding this comment

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

Let's only create the media dir when we need it, i.e., right before storing a file.

@@ -187,8 +187,17 @@ def select_journal(self, action, title, message):
return

if action == 'saveas':
# TODO: read directory name from self.dirs
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's best to add a property Filesystem.media_dir(self): return os.path.join(self.data_dir, 'media').

@jendrikseipp
Copy link
Owner

@ruhugu I'm planning to format the RedNotebook source code with black. This would mean you'll have to format your patched version with black as well, complicating things a bit. Or should I just wait until we merge your code?

@ruhugu
Copy link
Author

ruhugu commented May 12, 2020

Sorry about the late reply. Yes, proceed with the formatting. I will reformat my patched version.

This new function does not check if there is a file with the same name
in the destination folder.
With this we can avoid writing "media" explicitly several in parts of the
code.
@jendrikseipp
Copy link
Owner

@ruhugu Are you still planning to pursue this further?

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.

Relative paths and option to copy pictures and linked files into data directory
2 participants