Post here if you have re-based and finalised code to integrate into master, which was discussed, agreed to and tested in other forums. You can also submit your PR directly on github.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
uwestoehr wrote: ↑Fri May 15, 2020 3:22 pm
- the angle "0°" is allowed despite it is invalid
How do you work around this? Set the minimum value to something like FLT_MIN?
uwestoehr wrote: ↑Fri May 15, 2020 3:22 pm
- the angle does not check if the result will be invalid or not: Take e.g. a 5mm cube and add a 80° chamfer to see that this is invalid because this would be a distance of > 5mm. I am not sure this is easy to fix.
Well, I noticed the chamfer Build() documentation says this:
Warning The construction of chamfers implements highly complex construction algorithms. Consequently, there may be instances where the algorithm fails, for example if the data defining the parameters of the chamfer is not compatible with the geometry of the initial shape. There is no initial analysis of errors and these only become evident at the construction stage.
armandas wrote: ↑Sat May 16, 2020 12:43 pm
How do you work around this? Set the minimum value to something like FLT_MIN?
No, the minimum should be 0 deg and the fillet feature must raise an error if it's not > 0. The dialog must handle this error and disallow to accept invalid input.
In general it's anyway bad practice to add a lot of code to check if an operation can be performed or not. Much better is to just perform it and then react on thrown exceptions or an invalid shape. Also because there are so many different cases so that we will never find all examples where it can fail.
This is just a short post to say that I have not forgotten about this.
I see that there is a flip property now added to control the side of the distance in the distance-angle version.
There is an already increasing number of properties which are not used in all modes.
Discussing about changing the persistence of properties, realthunder suggested that maybe dynamic properties would be more appropriate to achieve that.
I have never used dynamic properties, but I will try to make it work with them. We might them see what is better and we might learn a couple of things from it.
armandas wrote: ↑Sat May 16, 2020 12:25 pm
...
Also, while we're talking about UI, any idea how to align the input fields? It looks a bit messy now.
In the .ui file, you can add a QGroupBox and set it to something like grid, so you have left columns and right columns.
I tried using glid layout and it almost works. For some reason, the angle field is smaller than Size and Size 2, even though the widget type is the same...
adrianinsaval wrote: ↑Thu May 14, 2020 6:58 pm
Why is DressUp::onChanged called anyway if the property change is handled already?
AFAIU, OnChanged is intended to follow a "chain of responsibility" kind of design pattern. This means that when ChamferType changes, Chamfer is responsible for taking into account the change. Unless there is some kind of shared responsibility, there is indeed no need to call up in the hierarchy if the property is already handled. A return statement should follow updateProperties(). Good eye!
Hi Adrian,
I must correct my answer. In this case, there is piece of common code (relates to touching and recomputes) executed at the end of the responsibility chain relating to the status bits. For this reason, it is not possible to early return, unless you want this common code to be ignored for some reason. Take a look to void DocumentObject::onChanged(const Property* prop) in the calls to testStatus().
armandas wrote: ↑Sun May 17, 2020 9:29 am
... even though the widget type is the same...
Check the geometry property of both widgets. Set a large maximum value for horizontal size, try different things. In some configurations the widget should "grow" to occupy the entire available horizontal space.
I tried to mess a bit with the Qt layouts and options to make the fields aligned, but I could not get it to work well. I think I will go with the current layout.
I made a couple of small changes in the code and added a separate icon for the Flip Direction button. Other than the issue with the properties, I think this is ready for review. Please let me know if there is anything else I have missed or done wrong.
abdullah wrote: ↑Sun May 17, 2020 7:06 am
I have never used dynamic properties, but I will try to make it work with them. We might them see what is better and we might learn a couple of things from it.
