Fixes image constructor with a filename as argument, solves #3947 #3949
Conversation
nhthn
added
the
comp: Qt GUI
nhthn
added
the
bug
| @@ -89,7 +89,7 @@ View : QObject { | |||
| this.background = color; | |||
| } | |||
|
|
|||
| backgroundImage_ { arg image; this.setBackgroundImage(image) } | |||
| backgroundImage_ { arg image, tileMode=1, alpha=1.0, fromRect; this.setBackgroundImage(image, tileMode, alpha, fromRect) } | |||
There was a problem hiding this comment.
setter_ methods should only take a single argument.
.gitignore
Outdated
| @@ -25,3 +25,6 @@ cmake-build-* | |||
|
|
|||
| *~ | |||
| *.orig | |||
|
|
|||
| # VS code | |||
| .vscode | |||
There was a problem hiding this comment.
thanks! looks like this got committed accidentally.
after fixing, please rebase so the commit history is clean.
There was a problem hiding this comment.
Would not it be useful for people ? I am browsing the code with Visual Studio Code and it is adding this .vscode file at the root of the project. And I don't want to push it to the repo.
As you wish !
There was a problem hiding this comment.
it's a perfectly okay change, but it shouldn't go in the same commit as other stuff. if you filed a PR specifically for this change, it would probably be merged quickly.
sorry to be so fussy, but a clean commit history pays off in the long run.
There was a problem hiding this comment.
@geoffroymontel - @snappizz is right. Please avoid "drive-by" fixes. Also, please make sure your files end in a newline - it currently doesn't (you can see this in the diff view in the screenshot here). I believe git will complain if you try to commit a change with bad whitespace (trailing whitespace, missing final newline). If it doesn't on your machine, you should configure it so it does.
If you don't want to manually redo the commits it's possible to split commits using git rebase [git help rebase, "SPLITTING COMMITS" section]. I can help with that here, via email or slack.
| @@ -89,7 +89,7 @@ View : QObject { | |||
| this.background = color; | |||
| } | |||
|
|
|||
| backgroundImage_ { arg image; this.setBackgroundImage(image) } | |||
| backgroundImage_ { arg image; this.setBackgroundImage(image) } | |||
There was a problem hiding this comment.
this change shouldn't be here.. it looks like all you really need to do for this PR is make the single change to the other line in QImage.sc. I would recommend making that change and force pushing to update this branch, then making the gitignore PR separately. If you want help with that I can give more advice here, via email or slack.
geoffroymontel
force-pushed
the
topic/fixes-image-constructor-with-filename
branch
from
55b237a to
40e03e0
Compare
|
Thanks for your feedbacks @brianlheim and @snappizz, I used |
Purpose and Motivation
Fixes #3947
Types of changes
Bug-fix
Checklist
Remaing work
I should have added a test for the Image constructor but I am not familiar enough with testing yet. I will try soon.