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

#10: Antonio: Implemented a way to make moving pictures within their … #46

Merged
merged 2 commits into from Jul 24, 2017
Merged

Conversation

antonioalonzi
Copy link
Contributor

Hi Adrien,

I realised that the functionality #10 has already been implemented just after I did it myself.
My solution is very similar to the one provided by jdavidberger, with the difference that there's no need to press the CTRL button to move the image.

For more info see the description below.

Any reason why the other pull request wasn't merged yet?
Furthermore how long usually take for the update of the apt repository?

Kind regards
Antonio

Commmit Comment:
The Photo object has been updated with two extra properties: offset_w and offset_h which represent respectively the offset on the width and the offset on the height of a photo within their frame.
The offset value must be a number between 0 and 1:

  • 0: means the photo is not cut on the left (top) but is fully cut on the right (bottom)
  • 1: means the photo is not cut on the right (bottom) but is fully cut on the left
  • 0.5: is the default value and means the photo is centered
  • any other value betwen 0 and 1 will move the photo within the frame
    A public move method has been added to move the photo within the cell.

ImagePreviewArea.SWAPPING has been renamed as SWAPPING_OR_MOVING because when clicking, moving the mouse and realising will trigger two different actions:
- SWAPPING: if the release of the button is done on another cell
- MOVING: if the release of the button is done on the same cell
The method ImagePreviewArea.button_release_event() has been accordingly modified.

The method RenderingTask.resize_photo() has been modified to take the offset in account.

@antonioalonzi
Copy link
Contributor Author

antonioalonzi commented Feb 26, 2017

I guess in the previous solution you didn't like that the user needed to press the CTRL button to move the photo.
Then hopefully you'll like my pull request.

I added as well a commit to change the flake8 configuration. I think 80 characters for a line is very very short nowadays. What do you think?
I'm editing the code on a laptop with PyCharm and a 100 characters line looks great.
(I really didn't want to split a formula in two lines and made it more unreadable)

Let me know if you like it or you would like to not commit the tox.ini file.

Copy link
Owner

@adrienverge adrienverge left a comment

Choose a reason for hiding this comment

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

Hi @antonioalonzi,

Please excuse me for this very, very late answer. I hope you're still interested in contributing this feature to PhotoCollage.

I have a few comments on the code, once fixed it will be good to go!

.gitignore Outdated
@@ -1,3 +1,4 @@
.idea
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this (you can use a global gitignore git config --global core.excludesfile '~/.gitignore')

tox.ini Outdated
@@ -0,0 +1,2 @@
[flake8]
max-line-length = 100
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer the good old 80, sorry :)

self.collage.page.swap_photos(self.swap_origin.cell,
self.swap_dest.cell)
self.parent.render_from_new_collage(self.collage)
else:
# same cell: MOVING
move_x = (self.swap_origin.x - self.x) / self.swap_dest.cell.w
Copy link
Owner

Choose a reason for hiding this comment

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

I'm getting lots of these while moving images, can you fix it?

Traceback (most recent call last):
  File ".../photocollage/gtkgui.py", line 615, in button_release_event
    move_x = (self.swap_origin.x - self.x) / self.swap_dest.cell.w
AttributeError: 'NoneType' object has no attribute 'w'

…frame.

The Photo object has been updated with two extra properties: offset_w and offset_h which represent respectively the offset on the width and the offset on the height of a photo within their frame.
The offset value must be a number between 0 and 1:
  - 0: means the photo is not cut on the left (top) but is fully cut on the right (bottom)
  - 1: means the photo is not cut on the right (bottom) but is fully cut on the left
  - 0.5: is the default value and means the photo is centered
  - any other value betwen 0 and 1 will move the photo within the frame
  A public move method has been added to move the photo within the cell.

  ImagePreviewArea.SWAPPING has been renamed as SWAPPING_OR_MOVING because when clicking, moving the mouse and realising will trigger two different actions:
    - SWAPPING: if the release of the button is done on another cell
    - MOVING: if the release of the button is done on the same cell
 The method ImagePreviewArea.button_release_event() has been accordingly modified.

 The method RenderingTask.resize_photo() has been modified to take the offset in account.
@antonioalonzi
Copy link
Contributor Author

Hi Adrien,

I removed the .idea in the .gitignore and the changes to tox.ini.

I fixed the bug that was throwing errors.
I guess you already knew what the problem was :). Basically when dragging and dropping outside any other photos I was getting that exception as I did not check that was in a valid cell.

I hope is good to go now.

P.S. I forced push so you are going to get one only commit.
I clicked on update branch button above (which unfortunately did a merge of your branch into mine rather than a rebase... but I guess you are not bothered about that).

@adrienverge
Copy link
Owner

@antonioalonzi This is great, thanks!

@adrienverge adrienverge merged commit 6167032 into adrienverge:master Jul 24, 2017
@ojob ojob mentioned this pull request Nov 11, 2017
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.

None yet

2 participants