Bug 64575 - Photo Album: Better Image Layout
Summary: Photo Album: Better Image Layout
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: medium normal
Assignee: abhilash300singh
URL:
Whiteboard: target:5.3.0
Keywords: difficultyBeginner, easyHack, skillCpp
Depends on:
Blocks:
 
Reported: 2013-05-14 07:49 UTC by Samuel Mehrbrodt (allotropia)
Modified: 2017-02-14 08:57 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
How it should look like (1.02 MB, application/vnd.oasis.opendocument.presentation)
2013-05-14 07:49 UTC, Samuel Mehrbrodt (allotropia)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Samuel Mehrbrodt (allotropia) 2013-05-14 07:49:50 UTC
Created attachment 79286 [details]
How it should look like

Currently the Photo Album allow only to choose whether to keep the Aspect Ratio or not.
It should always keep the Aspect Ratio, and the following options should be implemented:
 - Crop (also known as Zoom, would use all the available space and cut the rest of the image)
 - Fit (current "Keep Aspect Ratio" option)

The Code lives here: http://opengrok.libreoffice.org/xref/core/sd/source/ui/dlg/PhotoAlbumDialog.cxx and the Dialog is here: http://opengrok.libreoffice.org/xref/core/sd/uiconfig/simpress/ui/photoalbum.ui

See attached presentation how the result should be.
Comment 1 Björn Michaelsen 2013-10-04 18:46:34 UTC
adding LibreOffice developer list as CC to unresolved EasyHacks for better visibility.

see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Comment 2 Robinson Tryon (qubit) 2013-10-23 17:14:35 UTC Comment hidden (obsolete)
Comment 3 Rishabh 2015-02-17 10:29:25 UTC
Any suggestions on how the crop dialog is expected to look ?Currently, there is no way to crop the image before inserting the image but most of the crop dialogs which I have seen, crops the image before inserting the image. Do we need to create an interface for cropping ? Previously I have worked on #64573 so I am quite familiar with the code and would like to give it a try.
Comment 4 Robinson Tryon (qubit) 2015-12-14 07:01:42 UTC Comment hidden (obsolete)
Comment 5 Samuel Mehrbrodt (allotropia) 2016-01-12 12:53:40 UTC
https://gerrit.libreoffice.org/#/c/14912 has some code, if someone wants to continue that.
Comment 6 Shona 2016-02-10 01:49:53 UTC
I am working with Beza for my software engineering class. I am getting started to fix this awesome bug! Any information and suggestion is welcome. Anyone who recently worked on this bug? DO you guys have any recommendation about the IDE which is compatible to Libreoffice. We are trying to use Vim but we are struggling to get the code on the text editor. Any help support and comment is more than welcome.
Comment 7 BezaM 2016-02-14 04:22:35 UTC
Hello,

I am a student working with Shona (the assignee). If I understand this problem correctly, we are looking to have a tickable option "crop" in the dialog just like "keep aspect ratio" and "Add caption to each slide", and once that option is selected, the user will be able to crop the image in the dialog box before inserting. 

Then, do there need to also be a visual preview of the image inside the dialog or a separate window, while the user does the cropping? (For example, like crop features in photo editors, with adjustable grids)

Thank you!
Comment 8 Samuel Mehrbrodt (allotropia) 2016-02-16 08:02:03 UTC
(In reply to BezaM from comment #7)
> I am a student working with Shona (the assignee). If I understand this
> problem correctly, we are looking to have a tickable option "crop" in the
> dialog just like "keep aspect ratio" and "Add caption to each slide", and
> once that option is selected, the user will be able to crop the image in the
> dialog box before inserting. 
> 
> Then, do there need to also be a visual preview of the image inside the
> dialog or a separate window, while the user does the cropping? (For example,
> like crop features in photo editors, with adjustable grids)

No, this isn't meant to provide a manual cropping dialog. Instead the two options should do the following:

* Crop (maybe a better name is Fill): The image should fill the whole slide area while keeping the aspect ratio. So part of the image might be invisible because it's out of the slide area
* Fit: The image should use all available space in the slide area, but should not stretch larger than it. So some blank space might show up with this option. This is already implemented with the "Keep Aspect Ratio" option.

Hope this clears things up a little?
Comment 9 BezaM 2016-02-17 01:28:32 UTC
Yes it does! Thank you for the response!
Comment 10 Robinson Tryon (qubit) 2016-02-18 14:52:12 UTC Comment hidden (obsolete)
Comment 11 BezaM 2016-03-18 01:50:21 UTC
(In reply to Samuel Mehrbrodt (CIB) from comment #8)
> (In reply to BezaM from comment #7)
> > I am a student working with Shona (the assignee). If I understand this
> > problem correctly, we are looking to have a tickable option "crop" in the
> > dialog just like "keep aspect ratio" and "Add caption to each slide", and
> > once that option is selected, the user will be able to crop the image in the
> > dialog box before inserting. 
> > 
> > Then, do there need to also be a visual preview of the image inside the
> > dialog or a separate window, while the user does the cropping? (For example,
> > like crop features in photo editors, with adjustable grids)
> 
> No, this isn't meant to provide a manual cropping dialog. Instead the two
> options should do the following:
> 
> * Crop (maybe a better name is Fill): The image should fill the whole slide
> area while keeping the aspect ratio. So part of the image might be invisible
> because it's out of the slide area
> * Fit: The image should use all available space in the slide area, but
> should not stretch larger than it. So some blank space might show up with
> this option. This is already implemented with the "Keep Aspect Ratio" option.
> 
> Hope this clears things up a little?

Hello,

My partner and I are finalizing our solution for this problem. We have one question though. What happens when the picture inserted is already bigger in dimension than the slide and the user has only checked the new FILL check-box (without checking Keep Aspect Ratio checkbox) we are assuming it will remain the same since it already fills the slide.

Will that be the case?

Thank you!
Comment 12 Samuel Mehrbrodt (allotropia) 2016-03-18 05:43:04 UTC
(In reply to BezaM from comment #11)
> Hello,
> 
> My partner and I are finalizing our solution for this problem. We have one
> question though. What happens when the picture inserted is already bigger in
> dimension than the slide and the user has only checked the new FILL
> check-box (without checking Keep Aspect Ratio checkbox) we are assuming it
> will remain the same since it already fills the slide.
> 
> Will that be the case?

Hi,

no, in that case the image should be resized in such a way that it still covers the slide, but as much as possible from the image is visible.
I would presume that the aspect ratio is kept in all cases. Maybe we should remove the "Keep aspect ratio" checkbox? I cannot see a use case where you would not want to keep the aspect ratio.

See also: https://support.cloudinary.com/hc/en-us/articles/202521222-What-is-the-difference-between-Fill-Fit-and-Limit-scaling-modes- (ignore "Limit")
Comment 13 BezaM 2016-03-21 15:21:21 UTC
(In reply to Samuel Mehrbrodt (CIB) from comment #12)
> (In reply to BezaM from comment #11)
> > Hello,
> > 
> > My partner and I are finalizing our solution for this problem. We have one
> > question though. What happens when the picture inserted is already bigger in
> > dimension than the slide and the user has only checked the new FILL
> > check-box (without checking Keep Aspect Ratio checkbox) we are assuming it
> > will remain the same since it already fills the slide.
> > 
> > Will that be the case?
> 
> Hi,
> 
> no, in that case the image should be resized in such a way that it still
> covers the slide, but as much as possible from the image is visible.
> I would presume that the aspect ratio is kept in all cases. Maybe we should
> remove the "Keep aspect ratio" checkbox? I cannot see a use case where you
> would not want to keep the aspect ratio.
> 
> See also:
> https://support.cloudinary.com/hc/en-us/articles/202521222-What-is-the-
> difference-between-Fill-Fit-and-Limit-scaling-modes- (ignore "Limit")

Hello,

Thank you for the reply.It looks like we have a working code for one picture per slide that fills the slide while keeping aspect ratio. Does this feature have to be implemented for multiple pictures per slide as well? 

Because, in that case, the pictures will be overflowing over the other pictures when we try to make them fill their portion of the slide while keeping their aspect ratio.

Thank you.
Comment 14 Samuel Mehrbrodt (allotropia) 2016-03-21 15:33:25 UTC
(In reply to BezaM from comment #13)
> Hello,
> 
> Thank you for the reply.It looks like we have a working code for one picture
> per slide that fills the slide while keeping aspect ratio. Does this feature
> have to be implemented for multiple pictures per slide as well? 
> 
> Because, in that case, the pictures will be overflowing over the other
> pictures when we try to make them fill their portion of the slide while
> keeping their aspect ratio.

Sure, that would be nice.
But you can surely submit the current state as a patch to gerrit to make sure you are on the right track etc.
This addition could also be done in a different patch.

Looking forward to your patch (please add me as a reviewer once pushed to gerrit).
Comment 15 BezaM 2016-03-31 12:29:26 UTC
Patch submitted here for review 

https://gerrit.libreoffice.org/#/c/23668/
Comment 16 BezaM 2016-04-01 15:27:37 UTC
Hi,

I am trying to submit the patch again from the master branch of my forked core repository (Like I did before). 

This is a new fork that I forked yesterday, so I am assuming I would be doing a first submit in stead of 'resubmit'

My gerrit setup is successful.

The changes are committed to my github repository (https://github.com/mogesb/core) and the repository is only 1 commit behind from the LibreOffice/master (as I am writing this)

While running ./logerrit submit master, I get the following error.

beza@beza-Latitude-E5420:~/core$ ./logerrit submit master

git push ssh://logerrit/core HEAD:refs/for/master
Counting objects: 14, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (14/14), done.
Writing objects: 100% (14/14), 1.80 KiB | 0 bytes/s, done.
Total 14 (delta 11), reused 0 (delta 0)
remote: Resolving deltas: 100% (11/11)
remote: Processing changes: refs: 1, done    
To ssh://logerrit/core
 ! [remote rejected] HEAD -> refs/for/master (you are not allowed to upload merges)
error: failed to push some refs to 'ssh://logerrit/core'

beza@beza-Latitude-E5420:~/core$ ^C


What am I doing wrong?
Comment 17 Samuel Mehrbrodt (allotropia) 2016-04-04 06:42:23 UTC
(In reply to BezaM from comment #16)
> While running ./logerrit submit master, I get the following error.
> 
> beza@beza-Latitude-E5420:~/core$ ./logerrit submit master
> 
> git push ssh://logerrit/core HEAD:refs/for/master
> Counting objects: 14, done.
> Delta compression using up to 4 threads.
> Compressing objects: 100% (14/14), done.
> Writing objects: 100% (14/14), 1.80 KiB | 0 bytes/s, done.
> Total 14 (delta 11), reused 0 (delta 0)
> remote: Resolving deltas: 100% (11/11)
> remote: Processing changes: refs: 1, done    
> To ssh://logerrit/core
>  ! [remote rejected] HEAD -> refs/for/master (you are not allowed to upload
> merges)
> error: failed to push some refs to 'ssh://logerrit/core'
> 
> beza@beza-Latitude-E5420:~/core$ ^C
> 
> 
> What am I doing wrong?

Hi,

Gerrit gives you the reason: "you are not allowed to upload merges". It seems you have a merge commit in your history. Please make sure to squash your changes into *1* single commit and upload it to gerrit again. When making changes you need to use the --amend option to amend the existing change instead of creating a new one.
The wiki has more details on this.
Comment 18 jani 2016-05-23 06:36:05 UTC
A polite ping, still working on this issue ?

Seems you have a patch ready, but only need to remove the merge commits

look at git --squash when marging, that should do the trip
Comment 19 jani 2016-06-23 06:50:28 UTC
Unassigning due to lack of work.

Please reassign it, if you still work on the patch.
Comment 20 Raj Sarkar 2016-07-23 05:45:56 UTC
In the "Crop" option, is it necessary to actually crop the image. I wrote something that only resizes the image, but doesn't crop the image. When you play the slide show, it really doesn't matter because you can't see the part which should ideally have been cropped. The added advantage I see in this method is, that this method is non destructive, meaning the Users will be able to adjust the position of the image according to their wishes. 

In case actual cropping is required, please point me what library should I use.

Thanks.
Comment 21 jani 2016-07-24 06:47:16 UTC
Setting status
Comment 22 Samuel Mehrbrodt (allotropia) 2016-07-25 07:02:33 UTC
(In reply to Raj Sarkar from comment #20)
> In the "Crop" option, is it necessary to actually crop the image. I wrote
> something that only resizes the image, but doesn't crop the image. When you
> play the slide show, it really doesn't matter because you can't see the part
> which should ideally have been cropped. The added advantage I see in this
> method is, that this method is non destructive, meaning the Users will be
> able to adjust the position of the image according to their wishes. 

No, the actual cropping is not required. Resizing is enough.
Comment 23 abhilash300singh 2016-08-20 10:13:26 UTC
I'm new and don't quite understand how to handle jenkins. Can someone please take a look at why the jenkins build is repeatedly failing. I was initially recommended to rebase the patch, but to no avail. Things build and run just fine on my system. If it matters, my build might be 2-3 weeks old.
Comment 24 abhilash300singh 2016-08-20 10:14:34 UTC
Forgot to mention the link - https://gerrit.libreoffice.org/#/c/27654/
Comment 25 Commit Notification 2016-09-07 18:03:35 UTC
Abhilash committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=511acc66f543b0200ebe70cf072e5a8db8677809

tdf#64575 Photo Album: Better Image Layout

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 26 Commit Notification 2016-09-08 07:45:44 UTC
Samuel Mehrbrodt committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=64a6083df40c5ffe43333f557a96846ea4fa10f6

Related tdf#64575 Only allow "Fill Slide" for 1 Image/Slide

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.