class Handle Fix allowing dict to be passed for handlePen and removing hard reset for width and cosmetic attributes#2907
Conversation
|
New PR removed all the unnecessary changes to other files. |
|
The cosmetic=True can be dropped because pg.mkPen defaults to making cosmetic pens. From what I understand, pen widths of 0 are a holdover from Qt4 days. It is supposed to be equivalent to width=1 and cosmetic=True; in which case, even the width=0 can be dropped because pg.mkPen defaults to width=1. That said, leaving width=0 is the safe option. Note: pen widths of zero forces code that need to know about pen widths to handle the special case of zero. |
Places that convert 0 pen width to 1:
Places that failed to convert 0 pen width to 1: |
|
Considered there is a single failure maybe removing the 0 and setting to 1 is best? |
|
New commit removing those unnecessary default attributes values, I tested on my own app and worked perfectly. |
|
Hi @pdmkdz, thanks for the PR, sorry this has sat idle so long. The main issue I have with this PR is that we are specifying a mutable object as a default parameter in a function declaration. This is one of the main python gotchas which can cause really confusing behavior. Throughout the library it's common to use an immutable object (boolean, None, etc) and have an if-conditional that assigns the appropriate attribute to the dictionary. I think passing in a |
|
I removed the dict though in the last iteration.
Didn't the commit update?
I just removed the hard set to width=0, that is pretty bad because whatever
I pass in mkPen get overridden.
*Michele Del Zoppo*
M.Sc. Structural Geology & Geodynamics
Cell: +1 (336) 456 8213
Houston, TX
* [image: LinkedIn Account]
<https://www.linkedin.com/in/michele-del-zoppo-a8791761> *
*“An investment in knowledge pays the best interest.” **— Benjamin Franklin*
…-----------------------------------------------------------------------------------------
This message may contain confidential and privileged information. If it has
been sent to you in error, please reply to advise the sender of the error
and then immediately delete it. If you are not the intended recipient, do
not read, copy, disclose or otherwise use this message. The sender
disclaims any liability for such unauthorized use.
Il giorno mer 14 feb 2024 alle ore 05:40 Ogi Moore ***@***.***>
ha scritto:
Hi @pdmkdz <https://github.com/pdmkdz>, thanks for the PR, sorry this has
sat idle so long.
The main issue I have with this PR is that we are specifying a mutable
object as a default parameter in a function declaration. This is one of the main
python gotchas
<https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments>
which can cause really confusing behavior.
Throughout the library it's common to use an immutable object (boolean,
None, etc) and have an if-conditional that assigns the appropriate
attribute to the dictionary. I think passing in a dict to that argument
is fine, it just can't have a dict as a default value.
—
Reply to this email directly, view it on GitHub
<#2907 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANUM3SO4O3OTQHPHWQFOQJDYTSPBFAVCNFSM6AAAAABBANAKDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBTGU4TOMRXHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
No, there's still a {} dictionary. def __init__(self, radius, typ=None, pen={'color': (200, 200, 220)}, |
|
Hi @pdmkdz Thanks for the PR, I hope you don't mind me adding a commit to your branch, I wanted to get your PR into the next release, so I thought I would just make the small change myself. Thanks for your contribution to the library! |
|
Happy to help, my very first around here 😊 |
Adding ability to modify
handlePenforclass ROIremoving hard coded defaults formkPenwidth=0andcosmetic=Truein ROI.pyclass Handle. Keepingwidth=0andcosmetic=Trueas default for assignation without customization.Fixes #1960
Fixes #2765
This will allow to pass a dictionary for pen customization when needed due to visibility.