[bug] Cant add reference constraint to already defined circle/arc

Post here for help on using FreeCAD's graphical user interface (GUI).
Forum rules
and Helpful information
IMPORTANT: Please click here and read this first, before asking for help

Also, be nice to others! Read the FreeCAD code of conduct!
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: [bug] Cant add reference constraint to already defined circle/arc

Post by abdullah »

openBrain wrote: Sun Jun 13, 2021 6:24 pm
chrisb wrote: Fri Jun 11, 2021 10:13 pm Great addition. For me all three options are an improvement. For keeping as much as possible in its current state I would go for option 1 or 3.
abdullah wrote: Sat Jun 12, 2021 5:14 am I like it. Merged. Thanks. :D

1 or 3 is ok with me. In addition, you may make the new tool the initial default (before the new parameter has a given value).
Thx guys. I implemented option 3. ;) Pull request is here along with a bugfix for the previous PR => https://github.com/FreeCAD/FreeCAD/pull/4861
I found the "-1" in:
"int curRadDiaCons = hGrp->GetInt("CurRadDiaCons", -1);"

misleading for a code reader.

I see that the switch/case will filter this via "default", but still it is unexpected.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [bug] Cant add reference constraint to already defined circle/arc

Post by openBrain »

abdullah wrote: Sat Jun 19, 2021 6:31 am I found the "-1" in:
"int curRadDiaCons = hGrp->GetInt("CurRadDiaCons", -1);"

misleading for a code reader.

I see that the switch/case will filter this via "default", but still it is unexpected.
Humm. In my own programming paradigm, this is quite standard for 'undefined' (among other depending on the context). :? I'm not against changing that but can't think about something I'm sure is better. May you have a proposal?

Edit : not trying to convince but see how -1 is often found in Qt for similar meaning : https://doc.qt.io/qt-5/qtabbar.html#currentChanged
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: [bug] Cant add reference constraint to already defined circle/arc

Post by abdullah »

openBrain wrote: Sat Jun 19, 2021 8:52 am
abdullah wrote: Sat Jun 19, 2021 6:31 am I found the "-1" in:
"int curRadDiaCons = hGrp->GetInt("CurRadDiaCons", -1);"

misleading for a code reader.

I see that the switch/case will filter this via "default", but still it is unexpected.
Humm. In my own programming paradigm, this is quite standard for 'undefined' (among other depending on the context). :? I'm not against changing that but can't think about something I'm sure is better. May you have a proposal?

Edit : not trying to convince but see how -1 is often found in Qt for similar meaning : https://doc.qt.io/qt-5/qtabbar.html#currentChanged
What about "2"?

If I am understanding it correctly, we want that when the parameter does not exist, it defaults to "2" i.e. your improvement.

As I see it, we do not want to default it to "undefined", we want to default it to "your method". Am I not seeing something?
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [bug] Cant add reference constraint to already defined circle/arc

Post by openBrain »

abdullah wrote: Sat Jun 19, 2021 11:32 am What about "2"?

If I am understanding it correctly, we want that when the parameter does not exist, it defaults to "2" i.e. your improvement.
'2' will functionally do the same ATM. My idea was to be more generic in case there is an evolution in the future. ;)
I can update the PR in the coming hours if you think '2' is the way to go. ;) Just tell me.

Edit : just saw you already merged. So I can create a new PR to change if needed, or of course you can feel free to do it by yourself. :)
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: [bug] Cant add reference constraint to already defined circle/arc

Post by abdullah »

openBrain wrote: Sat Jun 19, 2021 11:49 am
abdullah wrote: Sat Jun 19, 2021 11:32 am What about "2"?

If I am understanding it correctly, we want that when the parameter does not exist, it defaults to "2" i.e. your improvement.
'2' will functionally do the same ATM. My idea was to be more generic in case there is an evolution in the future. ;)
I can update the PR in the coming hours if you think '2' is the way to go. ;) Just tell me.

Edit : just saw you already merged. So I can create a new PR to change if needed, or of course you can feel free to do it by yourself. :)
How I see it is that no matter where we might be heading in the future, we need a default value, which may be the current one or another one. I am happier with the parameter taking a value that is valid within the code, because the code shows this intent.

It was not my idea to have you change just one value. I did not merge it in the morning because I still had to compile it and run it and I was running out of time. I have now added a commit making the default '2'.

Thanks for the code :)
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [bug] Cant add reference constraint to already defined circle/arc

Post by openBrain »

abdullah wrote: Sat Jun 19, 2021 12:01 pm It was not my idea to have you change just one value. I did not merge it in the morning because I still had to compile it and run it and I was running out of time. I have now added a commit making the default '2'.
I'm very fine with having a shepherd for FC code and having my code reviewed. :) And I'm fine with just having to change a single value too. Finally it means my code was pretty good. :lol:
Thanks for the code :)
Thanks for the merge. I'll go for the documentation now. :)
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [bug] Cant add reference constraint to already defined circle/arc

Post by openBrain »

Documentation is now available : Sketcher ConstrainRadiam

I also fixed in documentation multiselection behavior that was changed by 1st PR of this topic regarding arcs/circles.
Post Reply