[Fixed] Recent Change to Selection? maybe Boost::Signals2??

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
User avatar
wandererfan
Veteran
Posts: 6268
Joined: Tue Nov 06, 2012 5:42 pm
Contact:

[Fixed] Recent Change to Selection? maybe Boost::Signals2??

Post by wandererfan »

Up until recently, these two code snippets would quite happily return TechDraw Views, Dimensions, Faces, Edges, Vertexes, etc from the QGraphicsView (Page). Now they return zero objects. Was there a change to how Selection works recently?

Code: Select all

s = FreeCADGui.Selection.getSelectionEx()

std::vector<Gui::SelectionObject> selection = cmd->getSelection().getSelectionEx(); 
We used to get a message in the status line (xxxxx.View.Edge1) too. That is also gone.

EDIT: changed title to include possibility Boost;:signals2 is involved.
EDIT: removed [solved] from title.
Last edited by wandererfan on Sat Nov 03, 2018 12:41 am, edited 2 times in total.
User avatar
wandererfan
Veteran
Posts: 6268
Joined: Tue Nov 06, 2012 5:42 pm
Contact:

Re: Recent Change to Selection? maybe Boost::Signals2??

Post by wandererfan »

Are we part way through a conversion from Boost::signal to Boost::signal2??

huxster@huxley ~/Source/FreeCAD-src $ git bisect good
f898eafd64f88902ea1916b01303705b3baa2c46 is the first bad commit
commit f898eafd64f88902ea1916b01303705b3baa2c46
Author: wmayer <wmayer@users.sourceforge.net>
Date: Tue Oct 30 19:09:03 2018 +0100

move from deprecated boost.signals to boost.signals2 library
User avatar
wandererfan
Veteran
Posts: 6268
Joined: Tue Nov 06, 2012 5:42 pm
Contact:

Re: [solved] Recent Change to Selection? maybe Boost::Signals2??

Post by wandererfan »

No idea why this commit caused selection to stop working, but it is working again in devel and a PR will be along shortly.
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: [solved] Recent Change to Selection? maybe Boost::Signals2??

Post by wmayer »

The reason for moving to boost.signals2 so quickly was that a FreeBSD user (or the package maintainer of FreeCAD on this platform) submitted a bug report where the future boost version will drop the deprecated boost.signals API and the release will be in December already.

Also we never used any complicated things of the old API so that the port was straightforward and inside the TechDraw module boost.signals2 was already used. So, for 99% of the code it was a matter of replacing the included header file with signals2.hpp and inside the class declarations replacing the name space.

So far there were only two places in the code where the blocking of signals must be handled differently: in App::Document and in SelectionObserver.
This is because the methods block/unblock have been moved to another class.

Running the unit tests didn't show any problems so I decided to commit and push the changes.

Sorry if this change is/was the reason for some issues.

Btw, I cannot confirm any problems with the selection. For me the above code snippets still return the expected results.
aapo
Posts: 615
Joined: Mon Oct 29, 2018 6:41 pm

Re: [solved] Recent Change to Selection? maybe Boost::Signals2??

Post by aapo »

Hi,

I was reading through the patch and was wondering why in the patch "bool ok" is taken from connectSelection?

Code: Select all

@@ -68,9 +67,9 @@ bool SelectionObserver::blockConnection(bool block)
{
    bool ok = connectSelection.blocked();

    if (block)
          blocker.block();
I would have thought that this should be taken from the new blocker, i.e., but apparently not?

Code: Select all

bool ok = blocker.blocking()
I mean, it would make more sense to me to the check for the blocking in the blocker itself. But it seems I'm wrong and misunderstood the actual locking mechanism, because wandererfan got it working again without touching the signals2 code itself, no?

Best regards,
Aapo
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: [solved] Recent Change to Selection? maybe Boost::Signals2??

Post by wmayer »

I would have thought that this should be taken from the new blocker, i.e., but apparently not?
Good question but shouldn't both have the same effect? I have to have a closer look at this...

At least the idea is to support nested calls:

Code: Select all

callFunction()
{
   bool ok = blockConnection(true); // ok should be true when called from myfunction
   ...
   blockConnection(ok); 
}

myfunction()
{
    bool ok = blockConnection(true); // ok should be false
    ...
    callFunction();
    ...
    blockConnection(ok); 
}
So, the first function call blocks and unblocks the connection and all nested function calls basically should have no effect.
aapo
Posts: 615
Joined: Mon Oct 29, 2018 6:41 pm

Re: [solved] Recent Change to Selection? maybe Boost::Signals2??

Post by aapo »

I agree that they should have the same effect, but I wasn't sure if both of the approaches actually worked correctly in practice. I was thinking that the object "connectSelection" would maybe not have any true information about the blocking state after the blocking semantics were put into separate object "blocker". But I tested both versions yesterday, and noticed no actual differences (both worked everywhere else but not in TechDraw), so I guess you are quite right about that.

Anyway, I'm glad that wandererfan got it sorted out somehow. I hope the patch will be available soon in github.
User avatar
wandererfan
Veteran
Posts: 6268
Joined: Tue Nov 06, 2012 5:42 pm
Contact:

Re: [solved] Recent Change to Selection? maybe Boost::Signals2??

Post by wandererfan »

wmayer wrote: Fri Nov 02, 2018 9:58 am

Code: Select all

callFunction()
{
   bool ok = blockConnection(true); // ok should be true when called from myfunction
   ...
   blockConnection(ok); 
}

myfunction()
{
    bool ok = blockConnection(true); // ok should be false
    ...
    callFunction();
    ...
    blockConnection(ok); 
}
FWIW, this is one of the things I changed to get TD working with the signals2 change. The change looks something like this:

Code: Select all

setTreeToSceneSelect(void)
{
//    bool ok = blockConnection(true); // ok should be false
    blockConnection(true);
    ...
//    blockConnection(ok); 
    blockConnection(false); 
}
Maybe it is a clue.
aapo
Posts: 615
Joined: Mon Oct 29, 2018 6:41 pm

Re: [solved] Recent Change to Selection? maybe Boost::Signals2??

Post by aapo »

I wrote a patch along my hypothesis, and the blocking seems to work with it.
FreeCAD_TechDraw_blockSelection.patch.txt
(7.63 KiB) Downloaded 41 times
There are few things to note:
  • I changed the blocking in Selection.cpp slightly, but as wmayer said it's probably not necessary.
  • There was one case in TechDraw, where saveSvg() blocked but did not release the block. This should be fixed in any case.
  • I made blockConnection() and blockSelection() in TechDraw to follow the same nesting guidelines
  • I took the "if (isSelectionBlocked) { return; }" from wandererfan's patch
  • The patch is not clean with newlines and special chars, sorry. Something garbled with unix/dos charsets, probably.
That's about it, now the blocking should be ok as far as I understand. Which is possibly not enough for this. :lol:
Post Reply