chrisb wrote: ↑Thu Oct 14, 2021 1:15 am
Meanwhile it would be great if other power users could have a look at the new sketcher stuff
I don't exactly what you expect, but will try to help.
Regarding new feature, I think it's a good feature but doesn't like much the implementation.
I'd prefer that there is a checkbox in front of "Filter" to enable/disable filtering, disabled by default, all associated settings hidden.
* When you enable filtering by checking the box : associated settings become visible. Settings previously used in the same sketch editing session are preserved (they are lost as soon as sketch editing mode is exited).
* When you disable filtering by unchecking the box, everything is back visible as it was before enabling filtering.
I don't like the "Show only filtered" to be present at 2 places ("Settings" + "Restrict visibility"). Also to me it consumes a lot of space, and also so strongly impact behavior that it shall be immediately visible. So I would implement it just as a toggle button with a symbol (for example an eye and a list) placed near left the "Settings" button. As it is a toggle button, user immediately sees if it's active or not.
"Select multiple" should IMO be enabled only if "Multiple" is selected as a filter.
The way I understand it, PRs are used in a twofold manner
1) To provide code changes from people who cannot merge their changes directly to the code.
2) To provide code changes to (a single or few) other developers for what reason so ever.
1) is totally right.
For 2), I guess it's up to the power developer (one that has merging rights) to decide if he/she shall do a PR or merge directly.
I try to review the PRs as much as I can, but I lack time currently. So what I do is to focus my review on code reading trying to find things that can not (or hardly) be found by user doing functional tests : under/overflows, unneeded processing, boundary conditions, suboptimal Qt usage, ...
Up to now, it seems pretty useful as I rarely found nothing. But it admittedly wouldn't have been super harmful if code has been merged as is.
I already gave my opinion in the past about increasing community feedback and inclusion. I see 2 possibilities. In any case I find it totally wrong to think that users being able to compile and run PRs is a satisfying user base to say that it represents community.
1) Endorse that FreeCAD dev version is highly stable. In this case, another version (let's call it "greenhouse", I like this word
) should be created and proposed to the users. This is a lot of work. Having a first filter that ensure PRs aren't unsafe, merge and build for all OSes, then have a second merging process for dev version, ... It looks like a cool idea, but I think it's too much effort for the contributing team right now.
2) Admit that enforcing a super stable dev version isn't so good as it limits pace at which PR are merged and at which community feedback is available. So we can lighten the PR review process (rely on the CI + a human review focusing on fact that code is safe and well written), then "blindly" merge to get more feedback. IMO not so critical as commits can generally be easily reverted if needed. Because of course some users like to have latest features but in a stable release, we could increase pace at which major/minor stable versions are released. This is IMO the way to go ATM.