-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
gr-qtgui: Add numeric entry widget #7349
base: main
Are you sure you want to change the base?
Conversation
Please look into |
I'm aware of the security implications of
The "normal" Entry widget also uses |
I haven't looked at the code, but I think this looks very neat UX-wise. |
Other places in GRC that do |
As mentioned before, QT GUI Entry is a counterexample: when type is set to raw, it calls
@willcode Yes, but that helps nothing if you are really concerned about arbitrary code execution. Here's an example for "some weird code" which – if passed to string = "[x().load_module('os').system('shutdown --help') for x in ().__class__.__base__.__subclasses__() if x.__name__=='BuiltinImporter']" I added On the other hand, to bypass the filter I am using here, you would need a local or global name which consists of no other letter than if match := re.search(r'[^0-9.e+\-*/() ]', string):
raise ValueError("Characters not allowed: " + match.group())
value = float(eval(string)) I consider this save If you want it bullet-prove and avoid import ast
def save_numeric_eval(expression):
save_operations = {
ast.UAdd: lambda a: a,
ast.USub: lambda a: -a,
ast.Add: lambda a,b: a+b,
ast.Sub: lambda a,b: a-b,
ast.Mult: lambda a,b: a*b,
ast.Div: lambda a,b: a/b,
ast.Pow: lambda a,b: a**b,
}
def evaluate(node):
if isinstance(node, ast.Expr):
return evaluate(node.value)
if isinstance(node, ast.Constant):
return node.value
if isinstance(node, ast.UnaryOp):
if op := save_operations.get(node.op.__class__, None):
return op(evaluate(node.operand))
if isinstance(node, ast.BinOp):
if op := save_operations.get(node.op.__class__, None):
return op(evaluate(node.left), evaluate(node.right))
raise TypeError("Unsupported operation: %s" % node.__class__.__name__)
return float(evaluate(ast.parse(expression).body[-1])) |
The eval should at least use a local namespace. I don't know to what degree that helps, but let's not be less "secure" than the rest of the system. Agree that there are ways around things, and there are other places in the system (e.g., embedded python blocks) that are worse. The Need the DCO/signoff on commits. Numbers are always floats, shown in exp format, which is generally not what the user wants to see. For example, if this is used for Hz, most people would expect to see @haakov @dl1ksv if you could try this out and give some feedback, that would be great. Should we just try to improve the existing entry block to support the features implemented here? |
Signed-off-by: Philipp Niedermayer <eltos@outlook.de>
Signed-off-by: Philipp Niedermayer <eltos@outlook.de>
Not sure what else I can say to show that it helps nothing in terms of security; you have to rigorously filter the string (like it did) or use AST (which I did now to put your mind to ease).
Did signoff, apparently your bot does not like my GitHub usernames. Both should be fixed now.
Agreed for labels, but not for entry fields. Imagine a field with unit "Hz" set to "30 Hz". The user types in "1000" which becomes "1 kHz". Then the user types in "1e6". Now, should this be interpreted in the base unit "Hz" and become "1 MHz" or in the previously shown unit "kHz" and become "1 GHz"? Either would be confusing from UX point of view. If the base unit is set to e.g. "MHz" then it could even be either of "Hz", "kHz" or "MHz". Sticking to a single unit avoids this ambiguity. And we don't want to force the user to type out the unit explicitly every time. |
Just played a bit with the example. That's what I got Traceback (most recent call last): SyntaxError: invalid syntax SyntaxError: invalid syntax Sorry, I don't know what I did. |
So, would it be possible to integrate the advantages of this block into the existing range or entry block, so we don't have multiple blocks that do almost the same thing? One difference is that this block is Python-only. |
Builds, install, works fine. We do have a number of qtgui widgets that are Python-only, so that's only a minor point. Since the range block is also Python-only, I think we should add this functionality to the range block, rather than add another block. Thank you for the AST calculator version of the code. Points well taken on the lack of security of other evals. |
Take the digital number control widget into consideration, too. |
It would make sense, but Range also supports integers and has sliders, knobs and counters which one would have to handle properly (one important feature of this block is that ranges are runtime adjustable and optional). Also, this blocks widget reflects "external" changes of the variable done by other blocks, which Range currently does not. |
@dl1ksv at least for my block you can simply use a Message Pair to Var at the Frequency Sinks message output to update the variable. My widget recognizes such "external" changes of its variables value and will update the text accordingly. This also works for Entry widget btw (but not for Range widget). |
@eltos I see no message domain in your yaml definition. So your block has no message ports. |
The idea above was that this would be done externally with msg-to-var. A msg port would be cleaner, though. |
@willcode Can you provide a hint for msg ports on variable-defining blocks?
which of course will fail because self.freq is the variable, not the block. It looks to me that a block which defines a variable and at the same time has ports can currently not be implemented in a clean way with GR, as its I guess this makes for a dedicated issue, but in the present context using a separate Message Pair to Var block seams much cleaner, actually. |
Description
The numeric entry widget combines the useful features of Range and Entry widgets and adds support for unit handling, numeric expressions in any common format and lower/upper bounds.
Examples (unit Hz):
When the entry is focussed, the user can use the UP/DOWN keys to in-/decrement the value by the specified increment, and the PageUp/PageDown for ten times the increment.
The label color is used to indicate the status:
blue = editing but value not yet applied
red = invalid input
black = value applied
If the variable is changed from some other place in the flowgraph via the top block's "set_xxx()" callback, or if one of the boundaries (min/max value) changes, this is reflected on the UI
The precision defines the maximum decimal places to consider.
Related Issue
none
Which blocks/areas does this affect?
new block in gr-qtgui
Testing Done
In my own OOT. Added an example flow graph to this PR:
gr-qtgui/examples/qtgui_numeric_entry.grc
Checklist