krathjen / studiolibrary

Studio Library
https://www.studiolibrary.com
GNU Lesser General Public License v3.0
354 stars 141 forks source link

Add support for changing the icon after an item has been created #17

Open krathjen opened 7 years ago

krathjen commented 7 years ago

Add support for changing the icon after an item has been created.

hammenm commented 7 years ago

I created a branch named feat/change-image-item with this feature.

You can look it up, change it, and give your opinion on it.

krathjen commented 7 years ago

Thanks! I'll try and have a look today! :)

hammenm commented 7 years ago

Ok! :) A minor issue that I observe is that the icon is not displayed when entering and leaving the widget with the mouse. I know it is a normal behavior because ImageSequence only pause the animation when the mouse leaves the widget. It might be of interest to have it have the image of the custom icon by default (maybe).

krathjen commented 7 years ago

Hey Hammen,

Thanks for looking at this issue! I think many people will appreciate having this option.

After having a quick look I have noticed some issues with the item not clearing the icon cache properly. I'll fix this issue in the core code and commit the changes.

I like the popup menu with the current options. What do you think about making this a class? Please see the attached file.

Thanks again! :)

Kurt

thumbnailcapturemenu.txt

import studiolibrary
import mutils.gui.thumbnailcapturemenu
mutils.gui.thumbnailcapturemenu.testThumbnailCaptureMenu()
hammenm commented 7 years ago

The class ThumbnailCaptureMenu will be quite helpful, and it will make the code clearer.

It is true that I don't have a global vision of the project yet, and I may not have implement the feature where it were supposed to be in the structure, hence the little bugs it have introduced.

Tell me if you want to change the code yourself, or if I should have a look into it.

Thank you for your feedback!

Edit: Also I might not be necessary to have the argument QtGui.QCursor.pos() when executing the menu. From what I understood, the menu origin is at the bottom of the button, like a dropdown menu.

krathjen commented 7 years ago

It would be great if you can update this, however, I'm happy to as well.

Sorry that the documentation is limited and the project still has a lot of improvements to be made. Having more people contributing will hopefully help.

I might split the baseitem.py module into baseitem.py, basecreatewidget.py and basepreviewwidget.py. This should hopefully make it clearer as well.

Edit: Also I might not be necessary to have the argument QtGui.QCursor.pos() when executing the menu. From what I understood, the menu origin is at the bottom of the button, like a dropdown menu.

I noticed that. I think that the dropdown menu works well. :)

Cheers,

Kurt

hammenm commented 7 years ago

Ok, I will update the code.

I'm pretty new to the git flow. Can you confirm that I should wait that you merge your recent code for cleaning the icon cache into the master branch, and then I update the pull request ?

And no problem if the code is uncomplete, I still find the project very useful for people who work with Maya :)

krathjen commented 7 years ago

yes, that is correct or you could rebase to the "fix/icon-cache branch". But I think it would be better if I remove the branch "fix/icon-cache" and make the changes directly to your branch "feat/change-image-icon". I think this is better for testing in case we need to change something else before merging it to master.

I'll update the changes soon.

krathjen commented 7 years ago

Hey Hammen,

I've updated the branch with the changes for clearing the iconCache. Make sure you pull the change to your machine.

Let me know how it goes.

Cheers,

Kurt

hammenm commented 7 years ago

Hello,

I did the commit (be7bb1d) to add the ThumbanilCaptureMenu and refactored the code to connect the widgets togther. However, it now fails to change the icon.

Does it need now to call combinedwidget to redraw the icon ? If you can, you may want to have a look into my code. I am not sure if I will have time to complete this feature soon.

krathjen commented 7 years ago

Thanks Hammen, I'll try and have a look today. :)

krathjen commented 7 years ago

Hi Hammen,

Do you mind if I make some big changes to that branch? There is nothing wrong with your code, I just think we may need to refactor a few things to get this working properly. Please let me know.

Thanks again for helping to bring this feature to life. :)

hammenm commented 7 years ago

Feel free to change whatever you want :)

krathjen commented 7 years ago

Hey Hammen,

I have pushed my changes to the "feat/change-item-image" branch. I have also squashed the commits and have rebased the branch to the latest master commit, so make sure you do a git pull for that branch and the master branch.

If you get a chance it would be great if you can test the changes.

Note: I have not added support for changing the AnimItem thumbnail yet.

Cheers,

Kurt

hammenm commented 7 years ago

Hello Kurt,

I see you have made a lot of changes. I will have a look on everything tomorrow, and hopefully add the feature.

krathjen commented 7 years ago

Okay, great! :)

hammenm commented 7 years ago

Hi,

I had time to look into the feature and all seems to work 👍 .

I just would like to ask : Does this feature should also be available for animitem, or should it be for poseitem only ?

Also I notice something, which may not be a bug, but is kind of surprising : If the new image resolution is 250x250 (or above?) the image will take all the space available, if the resolution is lower, then the image will have the size of the initial resolution, and the image is smaller. It is a normal case I think, because an image should not be displayed at a higher resolution than the initial resolution.

See you.

krathjen commented 7 years ago

Thanks Hammen for finding the time to test the changes.

It would be nice to add support for the animitem, however, I'm a bit busy over the next few days. I think we just need to add a way to set the "startFrame" and "endFrame" within the thumbnailcapturemenu.py. Or we could add these options to the thumbnailcapturedialog.py and just show the dialog for the animitem.

... If the new image resolution is 250x250 ...

I'll have a look at the resolution issue when I have a bit more time.

Thanks again for your help and feel free to make any changes!

Kurt

hammenm commented 7 years ago

It would be nice to add support for the animitem, however, I'm a bit busy over the next few days. I think we just need to add a way to set the "startFrame" and "endFrame" within the thumbnailcapturemenu.py. Or we could add these options to the thumbnailcapturedialog.py and just show the dialog for the animitem.

You are right about the animation. I forgot about ImageSequence and I just tought that it was enough to capture one image, but it is true that a animation is an image sequence.

Thank you again for your fedbacks :)