adrienverge / PhotoCollage

Graphical tool to make photo collage posters
GNU General Public License v2.0
425 stars 70 forks source link

Saving a collage definition #35

Open pdo-smith opened 7 years ago

pdo-smith commented 7 years ago

Is it possible to save a photo collage definition so that one can later return and resume one's work? I would like to be able to load a previous photo collage, make changes to it, by for example, adding new photos, deleting some and changing the layout.

adrienverge commented 7 years ago

Hi @pdo-smith, no it's not possible, sorry.

pdo-smith commented 7 years ago

Such a pity. I love this software. I am quite tempted to exercise my python.

frankMilde commented 7 years ago

Are there any expected problems with just serializing the classes using pickles?

http://docs.python-guide.org/en/latest/scenarios/serialization/ http://www.diveintopython3.net/serializing.html

adrienverge commented 7 years ago

@frankMilde pickles are not a safe way to do it because it allows arbitrary code execution (in particular, a malicious file could take control of the user session).

pdo-smith commented 7 years ago

@frankMilde , I have done just that and pickled the classes. It works very well. I can save and reload collage definitions at will. I have added two new methods, get_definition() and save_definition(). Here is the heart of the code

    output = open(pickleFile, 'wb')
    pickle.dump(self.history, output)
    pickle.dump(self.history_index, output)
    pickle.dump(self.opts, output)
    output.close()

and pkl_file = open(pickleFile, 'rb') self.history = pickle.load(pkl_file) self.history_index = pickle.load(pkl_file) self.opts = pickle.load(pkl_file) pkl_file.close() self.render_preview() self.set_title("PhotoCollage | "+pickleFile)

Yes, there are security problems and I am still thinking about work-arounds.

pdo-smith commented 7 years ago

Other features I have added:

  1. Click on cell photo and hold - show file name as tooltip
  2. Ctrl-Click - mark a cell photo for in-situ replacement by dragging and dropping a new photo
  3. Shift-Click - display cell photo in photo viewer(eog in my case)
  4. Alt Gr-Click - edit cell photo in Gimp and replace cell photo.
frankMilde commented 7 years ago

@pdo-smith the other features are great. Why no merge requests for the manipulation features? As they are quite a few, would a right-click menu make sense?

Also, editing an image in shotwell is a bit simpler than using the mighty gimp. Could that be added as well?

frankMilde commented 7 years ago

@adrienverge: I assume you strive for the highest standart for this product, and thus have concerns using pickles. But since this is an open source project, I am sure users would be ok with having subpar security in exchange for the possibility to save a collage.

Why not do as the python maintainers do? They know there serialization feature is unsecure, yet they still included it and give an appropriate warning in the docs:

Warning The pickle module is not intended to be secure against erroneous or maliciously constructed data. Never unpickle data received from an untrusted or unauthenticated source.

@pdo-smith how do you serialize the actual image files? Do you use the comple image or just the file path? If it is the latter, than it wouldnt even make sense to "load" collages from other users, and would greatly diminish the risk ok malicious code execution from external sources.

adrienverge commented 7 years ago

But since this is an open source project, I am sure users would be ok with having subpar security in exchange for the possibility to save a collage.

I don't understand: it's OK to have security flaws in this program because it is open source?

Why not do as the python maintainers do?

Python maintainers surely don't use pickle on untrusted sources! The pickle module exists because it is useful in many cases, but not for opening files from potentially untrusted origins.

pdo-smith commented 7 years ago

@frankMilde , I serialise only the classes history, history_index and opts. These contain the collage definitions, only the image file names and not the image files. My code is not intended to share collages with other people.

If you were only ever saving and loading the definitions from your own PC there would be no security problem since your own definitions are trusted sources. But some people may be storing on a server and then immediately the pickle file can no longer be trusted.

The solution seems to be to add a cryptographic signature to the pickle file on writing and to verify it on reading. I will try this. This link gives an idea of how this could be done: https://www.cigital.com/blog/python-pickling/

pdo-smith commented 7 years ago

@frankMilde , I think the bigger objection to serialising the classes is that code changes, always and inevitably. By serialising the class you are baking the class structure into the collage definition. If a later version changes the class structure, it invalidates your earlier collage definition files. Then you need to introduce versions and check the versions. It gets complicated and messy. From the point of view of a clean, maintainable design it is not a good idea.

What I have done is a quick hack for my immediate needs and I find it really, really valuable. But I am making problems for myself further down the road which is a bad design practice. Adrien has to worry about that more than I do, so he would choose a different solution, possibly JSON.

pdo-smith commented 7 years ago

@frankMilde , "Why no merge requests for the manipulation features?" My code was a quick hack and I have been lazy! But I find them really useful.

"As they are quite a few, would a right-click menu make sense?" Yes, it would make a lot of sense. Again, I was lazy(and I like shortcut keys)!

"Also, editing an image in shotwell is a bit simpler than using the mighty gimp. Could that be added as well?" Taken care of. The editor and photo viewer are configurable.

Since I use Shotwell to manage my photos I have been toying with another idea.

Tag the photos in Shotwell that I wish to add to photocollage. Add an option to photocollage to read the tagged photos from shotwell's database. For now I just use Shotwell's export feature to place the photos in a collage folder where photocollage can read them. This is easy enough to do so I am not sure if this feature would be worthwhile. And it would create more maintenance problems further down the road when Shotwell make changes.

frankMilde commented 7 years ago

@adrienverge: Agreed. Sorry, my bad. I would just love to have this feat :).

@pdo-smith: On the serialization issue: Yes, the design issue introduced by using pickles and being locked-in to a class structure, wtih versioning, crypto and all that mess is a big deterrent. Going for JSON seems much cleaner.

On the other features: The integrated workflow with shotwell sound great. I was doing the same workflow manually. It was quite cumbersome.

Do you have a fork on your github page, so someone could look at your hacks and maybe polishing them to production quality?