wxPropertyGrid - wxPG_PROP_ACTIVE_BTN flag turns a string property into a password field (Issue #23856)
Ronny Krüger
Description
In my property grid I create a string property, make it read-only and add an editor button to it. Since the property is read-only the editor button is not usable until the wxPG_PROP_ACTIVE_BTN flag is set on the property. But when I set this flag, the field showing the string becomes a password field and shows the string as a number of asterisks.
wxPropertyGrid* grid = new...; wxStringProperty* p = new wxStringProperty("My Property", "prop_name", "My Value"); grid->AppendIn(section, p); grid->SetPropertyReadOnly(p); grid->SetPropertyEditor(p, wxPGEditor_TextCtrlAndButton); p->ChangeFlag(wxPG_PROP_ACTIVE_BTN, true); // this turns the value string into asterisks
The reason for this is commit 57a9247 (2023-06-03 - Fix wxPG_PROP_ACTIVE_BTN flag definition) where the actual value of wxPG_PROP_ACTIVE_BTN was changed from wxPG_PROP_CLASS_SPECIFIC_1 to wxPG_PROP_CLASS_SPECIFIC_2 in order to fix a value collision with wxPG_PROP_SHOW_FULL_FILENAME. The problem is that now wxPG_PROP_ACTIVE_BTN clashes with wxPG_PROP_PASSWORD which also has the value of wxPG_PROP_CLASS_SPECIFIC_2 (see progrid/private.h at the bottom).
Setting wxPG_PROP_ACTIVE_BTN to wxPG_PROP_CLASS_SPECIFIC_3 would probably fix this bug. But somehow this feels like a workaround. I don't really understand those CLASS_SPECIFIC enums. The way they are used in the code make it easy to mix them up and create collisions between flags (as proven by this bug). But I have no better suggestion how to fix this. Maybe somebody with more knowledge about the internals of wxPropGrid can propose a better solution (or maybe confirm that this is the best we can do).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Ugh... Thanks for reporting this and I agree that all this looks very hackish but unfortunately I'm in the same boat and don't know this code well, so I can't propose a better solution.
Hopefully @a-wi might have some idea but if not we're going to have to do it like this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AW
Unfortunately, implementation with wxPG_PROP_CLASS_SPECIFIC_* bit fields is legacy code to save number of bits occupied by many wxPGProperty class-specific wxPG_PROP_* flags.
In principle these flags are used internally by wxPropertyGrid in non-conflicting combinations only.
Using wxPGProperty::ChangeFlag() in the user code may cause issues because this function is intended mainly for internal purposes - https://docs.wxwidgets.org/trunk/classwx_p_g_property.html#a8cebe6f162709ab46c878f9c4e7b53a4.
As a fix for this particular issue setting wxPG_PROP_ACTIVE_BTN to wxPG_PROP_CLASS_SPECIFIC_3 should be fine.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
But we also have
#define wxPG_PROP_COLOUR_HAS_ALPHA wxPG_PROP_CLASS_SPECIFIC_3
So it looks like wxPG_PROP_ACTIVE_BTN would conflict with wxPG_PROP_COLOUR_HAS_ALPHA then instead.
Why can't we make wxPG_PROP_ACTIVE_BTN a full-fledged member of wxPGPropertyFlags with its own value instead?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AW
wxPG_PROP_ACTIVE_* flags shouldn't be used in the user code. Some of them (like wxPG_PROP_ACTIVE_BTN) are exposed in the public API for technical and backward compatibility reasons. Some of them are clearly private - e.g. wxPG_PROP_PASSWORD, wxPG_PROP_USE_CHECKBOX.
Modifying them directly in the user code may cause issues.
These flags are used to internally represent the status of the wxPGProperty and not to change its status and therefore shouldn't be modified directly by the user, e.g:
wxPG_PROP_COLOUR_HAS_ALPHA->wxPGProperty::SetAttribute( wxPG_COLOUR_HAS_ALPHA, ...)is intended to be called by the user.wxPG_PROP_PASSWORD->wxStringProperty::SetAttribute(wxPG_STRING_PASSWORD, ...)is intended to be called by the user.wxPG_PROP_READONLY->wxPropertyGridInterface::SetPropertyReadOnly(...)is intended to be called by the user.wxPG_PROP_DISABLED -> wxPropertyGridInterface::EnableProperty(...)is intended to be called by the user.- etc.
wxPropertyGrid takes care of using non-conflicting combinations of wxPG_PROP_* flags.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks for the explanation, but I'm still not totally sure if it is a potential problem or not to have the clash between wxPG_PROP_COLOUR_HAS_ALPHA (even if it's not used directly but by setting wxPG_COLOUR_HAS_ALPHA attribute) and wxPG_PROP_ACTIVE_BTN?
If yes, and if the latter shouldn't be part of wxPGPropertyFlags, do we need to add wxPG_PROP_CLASS_SPECIFIC_4?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AW
I think the future-proof solution would be to move `wxPGPropertyClass' to private.h
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Maybe, but for now, do you think the clash between wxPG_PROP_COLOUR_HAS_ALPHA and wxPG_PROP_ACTIVE_BTN is possible? I.e. should we apply the original proposal to redefine the latter as wxPG_PROP_CLASS_SPECIFIC_3 or not?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AW
Closed #23856 as completed via 87e86de.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ronny Krüger
I just got around updating to the current master of wx. But the fix that closed this issue did not really do what I expected.
Apparently wxPG_PROP_ACTIVE_BTN is now a private implementation detail. So my code is not compiling anymore.
I forgot to mention this in my initial issue description, but I am not using wx-3.2. And I don't want to define WXWIN_COMPATIBILITY_3_2 just to get wxPG_PROP_ACTIVE_BTN (and a deprecation warning) back.
For now I just declare wxPG_PROP_ACTIVE_BTN in my own code. But this is of course an evil hack.
I think that what I do (making a property read-only but being able to have a usable editor button) is not an unreasonable use case. So my question is if we could make this an official feature.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AW
Maybe in your particular case it works but the case is that using wxPG_PROP_ACTIVE_BTN in combination with other flags or in the other properties can lead to unexpected behavior (e.g in wxDirProperty) and that's why it was/is not intended to be used in the user code.
Again, wxPGProperty::ChangeFlag() you use to change the flag is intended for internal use - https://docs.wxwidgets.org/trunk/classwx_p_g_property.html#a8cebe6f162709ab46c878f9c4e7b53a4.
If you need to implement an editable string property try deriving from wxLongStringProperty - it's intended for these purposes.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ronny Krüger
I read the documentation in the header file but was (wrongly) encouraged to still use ChangeFlag() by the words mainly and almost. :-)
Maybe it would be a good idea to move the declaration of ChangeFlag() (and SetFlagRecursively()) further down in the header file. This way it would look less belonging to the official client API.
As for the unexpected behavior you mentioned when using this flag with other properties: I don't think that it is really unexpected when client code deliberately uses this flag on a property that does not support it and it does not work. :-)
But I agree that this flag API is so low-level that it is easy to shoot yourself in the foot.
That is why I asked if we could make a feature (not the flag) to enable or disable an editor button controllable from client code official. And I meant having a proper method for doing this.
Thanks for the suggestion to derive from wxLongStringProperty. I completely missed that class.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.