GNOME Bugzilla – Bug 377123
[eog-ng] implement EogJobSave
Last modified: 2007-03-10 01:22:59 UTC
I started to re-implement saving into the eog-ng branch. But I'd like to hear some opinions on my plans how to do it. :-) The implementation needs to be adapted to the new job system. For that I created the EogJobSave object a subclass of EogJob. This one is used to do the simple save operation on one image file. For the SaveAs operation I plan to subclass EogJobSave to make EogJobSave as. The current working implementation consists only of EogJobSave (see patch below). It has some issues: - I only tested it locally - It doesn't give any feedback on the progress yet. Do we want to keep the old progress dialog or should it use EogStatusbar? Keeping the status dialog would also partially solve the next problem: - There is no way of cancelling it. - Although it is prepared to save a list of files it's currently only "single-file". The problem here is that I'm not yet sure if we might get async issues (like the ones EOG currently has) when I have it use its own EogJobLoad. - There is a little issue with updating the thumbnail after saving the file. The image-loading icon is set but the thumbnail is not loaded. This is because the monitor event chain is DELETE->CREATE->CHANGED for that file, which seems to cause some problems for EogListStore. Well, these are the issues I'm currently aware of.
Created attachment 76861 [details] [review] my first draft implementation This is my first implementation as described above. I consider it as a draft.
Hey, > I started to re-implement saving into the eog-ng branch. But I'd like to hear > some opinions on my plans how to do it. :-) Here we go. > The current working implementation consists only of EogJobSave (see patch > below). > It has some issues: > > - I only tested it locally Ok, let's work on having the basic functionality working first then we can stress the implementation with some remote images tests. > - It doesn't give any feedback on the progress yet. > Do we want to keep the old progress dialog or should it use EogStatusbar? > Keeping the status dialog would also partially solve the next problem: > - There is no way of cancelling it. Ok, I'm not sure about it yet. At first sight, I thought about using the status bar (just like the image loading job) but there are some issues. Image loading job is for one image, saving is for multiple images. There should be a clear UI feedback about which images are being saved at a certain moment. Also, there should be a way to cancel the job at any point. One option would be to show a Cancel button on the right side of the progress bar in the status bar (similar to what Gimp does). This button should appear for image loading too. The status bar layout would be replaced by the progress bar + cancel button while the save/load jobs are active. I need some help from the usability guys though. > - Although it is prepared to save a list of files it's currently only > "single-file". > The problem here is that I'm not yet sure if we might get async issues (like > the ones EOG currently has) when I have it use its own EogJobLoad. The old implementation called eog_image_load inside the save job. I see no problem with this because it's of the save job to load the image content before actually saving it to disk. The image load inside save job doesn't need to be async if that answers your question. > - There is a little issue with updating the thumbnail after saving the file. > The image-loading icon is set but the thumbnail is not loaded. > This is because the monitor event chain is DELETE->CREATE->CHANGED for that > file, which seems to cause some problems for EogListStore. I'll investigate this. You can focus on saving issues.
Created attachment 77126 [details] [review] add saving multiple files at once This little update makes the job load the images it uses. This allows saving multiple files at once. I added a little comment to that line just in case som problems show up.
Created attachment 78452 [details] [review] included rough basic saveas support This one adds preliminary SaveAs support. Preliminary because it doesn't handle file overwrite and is yet one file at a time only. EogJobSaveAs is implemented as a subclass of EogJobSave so we don't need an extra queue for these. To achive this i made the save jobs "run"-part overridable (not sure if there is a better solution).
Created attachment 78878 [details] [review] near-complete core-functionality This patch now also does support "SaveAsMultiple". What is still missing? Progress reporting, cancelling and overwriting files. I don't know how to handle the first two yet. Regarding overwriting files I am a little undecided if I should do it using a simple callback or emitting a signal.
Hey Felix, I'm sure we should keep this SaveAsMultiple feature. I think this is a kind of inconsistent (and in some way "hidden") feature. IMHO, we should remove this feature from the core and make SaveAsMultiple a plugin in the near future. I'd like to receive some users/developers feedback about it though. Comments?
I don't think it is that inconsistent, if you see the possibility to select and save several images at once as its counterpart using the normal saving methods. There is room for improval though: 1. The biggest problem it has is that the dialog doesn't look like the Save/SaveAs dialogs which could be irritating to the user. Maybe it is possible to bring it in line with the others. 2. Set the "%f" token as the initial filename, so the user gets a hint of what is is going on. 3. Think about way of making the other tokens work. What should we do if an image doesn't have the necessary tags? 4. Improve the chapter about saving in the help manual. Currently it doesn't even tell you about the possibility to change your images and save all changes at once. Maybe this would help to make it a little more userfriendly und less "hidden". :) IMO, removing it now would make the whole UI (app?) a little more inconsistent, as there would be actions which apply to multiple images at once while there are some which apply to single images only. It shouldn't be a problem to make it a plugin later, though.
Created attachment 79283 [details] [review] added progress reporting This one adds progress reporting (for the ease of it uses the loadjob progress callback to display it in the statusbar. This needs testing though.
Felix, there's an interesting suggestion for a new name for this "Save As" dialog for multiple image in bug #383084. Discussion about this specific issue might take place there. Adding dependency.
Felix, After some changes in the EogImage API made by Lucas, your patch needs a little tweaking in order to build. Most notably, SIGNAL_PROGRESS was removed from EogImage, which was used to report progress. I'm giving some test to your work, will give some feedback hopefully later.
Created attachment 82171 [details] [review] updated patch Here is an updated patch. It is identical to the last one except that it readds the removed signal and applies to a current eog-ng branch. I didn't have time to test it extensively though.
Hi Felix, suggested changes/fixes in your patch: 1. Critical error when using the save as dialog for multiple images. Eog-ERROR **: file eog-save-as-dialog-helper.c: line 349 (eog_save_as_dialog_new): assertion failed: (data->nth_image >= 0 && data->nth_image < data->n_images) 2. The UI feedback for saving actions is not working. The save progress dialog doesn't show up. 3. For consistency reasons, you should use progress signal from EogJob to keep track of the saving progress. No need for a "saving-progress" signal in EogImage for that. If cannot work on this patch in the next 2-3 weeks, please let me know because we need to have saving actions working for the beginning of 2.19.x cycle. Thanks!
(In reply to comment #12) > Hi Felix, suggested changes/fixes in your patch: Hi Lucas! > 1. Critical error when using the save as dialog for multiple images. > > Eog-ERROR **: file eog-save-as-dialog-helper.c: line 349 > (eog_save_as_dialog_new): assertion failed: (data->nth_image >= 0 && > data->nth_image < data->n_images) Hmm, strange it works here. As far as I can currently see this could theoretically only happen if the list of selected images is empty. How many images did you select? > > 2. The UI feedback for saving actions is not working. The save progress dialog > doesn't show up. It doesn't use the dialog. It uses the progress bar to show whats going on. But this does not appear to work as I intended it to. > > 3. For consistency reasons, you should use progress signal from EogJob to keep > track of the saving progress. No need for a "saving-progress" signal in > EogImage for that. The EogImage:save-progress signal is used to show how far gnome-vfs has copied the saved file over as that is still done inside EogImage. This signal then reports to the save job which uses its own progress signal to give feedback about its progress. > If cannot work on this patch in the next 2-3 weeks, please let me know because > we need to have saving actions working for the beginning of 2.19.x cycle. Yes, it's going to be hard for me to do any real work in the next few weeks as I am writing tests at university the first 3 weeks of March. I'll see if I can manage to do some work between learning in the next days, but I can't promise as it was a pretty difficult semester for me. :-( > > Thanks! >
Working on that now.
Ok, done. Still needs a lot of polishing. The idea here is to have the very basic functionalities working already in eog-ng as I want to merge in trunk as soon as 2.19.x/2.20.x development cycle begins. Thanks Felix! 2007-03-10 Lucas Rocha <lucasr@gnome.org> Restabilished the image saving functionalities. Based on patch from Felix Riemann <friemann@svn.gnome.org>. Fixes bug #377123. A lot of polishing is still needed. * src/eog-exif-util.h: fix small typo and coding style. * src/eog-transform.c: added progress feeback for transformations. * src/eog-image.[ch] (eog_image_class_init, eog_image_real_transform, handle_xfer_status, tmp_file_move_to_uri): added a "save-progress" signal. Some coding style fixes. * src/eog-job-queue.c (remove_job_from_queue, handle_job, no_jobs_available_unlocked, search_for_jobs_unlocked, eog_job_queue_init), src/eog-job.[ch] (eog_job_save_*, eog_job_save_as_*): implementation of EogJobSave and EogJobSaveAs.