Connect slot to an int function !?

[hakermania]

Ok this is quite a noob question, I know. I have made this
constructor of mainwindow.cpp:

(void) new QShortcut(Qt::ALT + Qt::Key_1, this, SLOT(show_m(0)));
    (void) new QShortcut(Qt::ALT + Qt::Key_2, this, SLOT(show_m(1)));
    (void) new QShortcut(Qt::ALT + Qt::Key_3, this, SLOT(show_m(2)));
    (void) new QShortcut(Qt::ALT + Qt::Key_4, this, SLOT(show_m(3)));

(void) new QShortcut(Qt::ALT + Qt::Key_1, this, SLOT(show_m(0)));
    (void) new QShortcut(Qt::ALT + Qt::Key_2, this, SLOT(show_m(1)));
    (void) new QShortcut(Qt::ALT + Qt::Key_3, this, SLOT(show_m(2)));
    (void) new QShortcut(Qt::ALT + Qt::Key_4, this, SLOT(show_m(3)));

To copy to clipboard, switch view to plain text mode 

well, the slot show_m(int) looks like:

int MainWindow::show_m(int a)
{
ui->tabWidget->setCurrentIndex(a);
}

int MainWindow::show_m(int a)
{
ui->tabWidget->setCurrentIndex(a);
}

To copy to clipboard, switch view to plain text mode 

As you may understand, I want by pressing Alt+1/2/3/4 to navigate through the tabs. I don't get compile error but output error:

Object::connect: No such slot MainWindow::show_m(0) in megauploadMain.cpp:102
Object::connect:  (receiver name: 'MainWindow')
Object::connect: No such slot MainWindow::show_m(1) in megauploadMain.cpp:103
Object::connect:  (receiver name: 'MainWindow')
Object::connect: No such slot MainWindow::show_m(2) in megauploadMain.cpp:104
Object::connect:  (receiver name: 'MainWindow')
Object::connect: No such slot MainWindow::show_m(3) in megauploadMain.cpp:105
Object::connect:  (receiver name: 'MainWindow')

Object::connect: No such slot MainWindow::show_m(0) in megauploadMain.cpp:102
Object::connect:  (receiver name: 'MainWindow')
Object::connect: No such slot MainWindow::show_m(1) in megauploadMain.cpp:103
Object::connect:  (receiver name: 'MainWindow')
Object::connect: No such slot MainWindow::show_m(2) in megauploadMain.cpp:104
Object::connect:  (receiver name: 'MainWindow')
Object::connect: No such slot MainWindow::show_m(3) in megauploadMain.cpp:105
Object::connect:  (receiver name: 'MainWindow')

To copy to clipboard, switch view to plain text mode 

Thx in advance for any replies!

[high_flyer]

signal slots take on type parameters not values!

(void) new QShortcut(Qt::ALT + Qt::Key_1, this, SLOT(show_m(int)));

(void) new QShortcut(Qt::ALT + Qt::Key_1, this, SLOT(show_m(int)));

To copy to clipboard, switch view to plain text mode 

int MainWindow::show_m(int a)
{
switch(a)
{
 case 1: //do something 
   break;
 ....
}
 
}

int MainWindow::show_m(int a)
{
switch(a)
{
 case 1: //do something 
   break;
 ....
}

}

To copy to clipboard, switch view to plain text mode 

[johnc]

You can't hard code an input parameter value to a slot on connection. In order to implement what you are trying to do, use a QSignalMapper. You can associate your integer values with a mapping for each shortcut. Something like this:

QSignalMapper* mapper = new QSignalMapper(this);
//Do this for each shortcut, each with different mapping
QShortcut* s = new QShortcut(Qt::ALT + Qt::Key_1, mapper, SLOT(map()));
mapper->setMapping(s, 1);

//Finally, connect the mapper to your classes slot
bool ok = connect(mapper, SIGNAL(mapped(int)), this, SLOT(show_m(int)));
Q_ASSERT(ok); //I always perform this check to cause a debug assertion if I messed something up in the signal connection

[high_flyer]

johnc is right, but I don't see what use is the signal mapper here.
The signal mapper is good for when you have multiple dynamic, only in run time determined controls that need to be connected to slots by some rules.
But for finite number of known objects, I am not sure if using the a signal mapper is really less coding, or has any other advantage over regular old school connections.
You can implement the same functionality as the signal mapper by just connecting to one slot, and in the slot do:

void MyClass::mySmartSlot()
{
   switch(sender())
   {
     case pButton1: //do button stuff;
       break;
     case pButton2: // do button 2 stuff;
       break;
    //or even 
    case pLineEdit: //we can even do other control stuff with the same slot
      break;
   }
}

void MyClass::mySmartSlot()
{
   switch(sender())
   {
     case pButton1: //do button stuff;
       break;
     case pButton2: // do button 2 stuff;
       break;
    //or even 
    case pLineEdit: //we can even do other control stuff with the same slot
      break;
   }
}

To copy to clipboard, switch view to plain text mode 

This approach gives far more flexibility, and is not more coding.
But this is usually not needed.

[hakermania]

Thx both

[nroberts]

johnc is right, but I don't see what use is the signal mapper here.

The benefit of the signal mapper is to avoid violation of LSP. Every time they add a new shortcut/button/whatever to trigger a set_m event they would have to modify their original object to account for it. This isn't the correct way to design. You don't design interfaces to account for the implementation details of each and every client; you design interfaces to be as simple as possible and still complete. That's what the OP has here with set_m(), a function that in turn triggers a specific change within the state of the object.

At the very least you would want to put your switch function in something external to the set_m() object. At this point you're simply reinventing the wheel because QSignalMapper does exactly what is needed.

Code size is often a very poor metric when evaluating solutions. Sometimes it truly is necessary though even in embedded systems this is less and less of a consideration at all. A better metric is something that measures maintainability, and the Principles of OO Design (look them up) are more often than not the best metric I've seen. Still can't really be automated, needs intelligence to evaluate, but are well worth knowing about and using.

[high_flyer]

The benefit of the signal mapper is to avoid violation of LSP. Every time they add a new shortcut/button/whatever to trigger a set_m event they would have to modify their original object to account for it. This isn't the correct way to design. You don't design interfaces to account for the implementation details of each and every client; you design interfaces to be as simple as possible and still complete. That's what the OP has here with set_m(), a function that in turn triggers a specific change within the state of the object.

All is good an well in what you say - but this is not an interface design - this is and implementation, what we deal here with.
And you will always change the implementation when you change something.
So all the good things you spoke of, as true as they are, do not apply here.

In addition the rest of the things you said are true again, but for large scale proojects, with full production that goes to clients.
This here is a home project.

At the very least you would want to put your switch function in something external to the set_m() object.

The switch function is a downn scaled implementation to what the SignalMapper is doing, but tailored to the specific application.
It could be that it could be put better somehwere else, that was not the point I was making.

Again, SignalMapper has its merits, but also problems.
And they have to be weighed against the "design overhead".
In most "usual" cases, I still say, its not needed, and introduces more problems then it solves.

But, I didn't want to hijack the thread, different people have different opinions, we can agree not to agree

[nroberts]

And you will always change the implementation when you change something.

You're doing it wrong. See the Open Closed Principle.

In addition the rest of the things you said are true again, but for large scale proojects, with full production that goes to clients.
This here is a home project.

Poor design methodology will turn a home project into a total mess just as fast as a large scale project. "It's just home brew," is never a good excuse to cut corners.

The switch function is a downn scaled implementation to what the SignalMapper is doing, but tailored to the specific application.

An unnecessary one. You're missing the point of OO: reuse.

And they have to be weighed against the "design overhead".

And your method has the most. You forgot to measure maintenance in your complexity evaluation. Further you didn't take into account that there now needs to be TWO versions of set_m(), one that depends on there being a signal generator, the other that can be called by client code directly.

Do anyone know when this all changed and the case keyword accepted non-constant values?

It hasn't changed. They could change it to be based on some sort of ID though and I'd be saying the same thing. The pattern itself is flawed, the fact that it doesn't work as specified is just an implementation detail.

[wysota]

There is another clean solution to the problem. Since QShortcut is a QObject one can do any of these:

QShortcut *sh1 = new QShortcut(...);
sh1->setProperty("tabIndex", 0);
QShortcut *sh2 = new QShortcut(...);
sh2->setProperty("tabIndex", 1);
connect(sh1, SIGNAL(activated()), ..., SLOT(someSlot()));
connect(sh2, SIGNAL(activated()), ..., SLOT(someSlot()));
//...
void ...::someSlot() {
  QShortcut *sh = qobject_cast<QShortcut*>(sender());
  if(!sh) return;
  tabWidget->setCurrentIndex(sh->property("tabIndex").toInt());
}

QShortcut *sh1 = new QShortcut(...);
sh1->setProperty("tabIndex", 0);
QShortcut *sh2 = new QShortcut(...);
sh2->setProperty("tabIndex", 1);
connect(sh1, SIGNAL(activated()), ..., SLOT(someSlot()));
connect(sh2, SIGNAL(activated()), ..., SLOT(someSlot()));
//...
void ...::someSlot() {
  QShortcut *sh = qobject_cast<QShortcut*>(sender());
  if(!sh) return;
  tabWidget->setCurrentIndex(sh->property("tabIndex").toInt());
}

To copy to clipboard, switch view to plain text mode 

... or the same but with a pointer, or:

QMap<QObject*,int> map;
//...
map.insert(sh1, 0);
map.insert(sh2, 1);
//...
void ...::someSlot() {
  int v = map.value(sender(), -1);
  if(v!=-1) tabWidget->setCurrentIndex(v);
}

QMap<QObject*,int> map;
//...
map.insert(sh1, 0);
map.insert(sh2, 1);
//...
void ...::someSlot() {
  int v = map.value(sender(), -1);
  if(v!=-1) tabWidget->setCurrentIndex(v);
}

To copy to clipboard, switch view to plain text mode 

or again with pointers to widgets...

Clean, reusable, object oriented. The only thing that hurts is the use of sender() but since QSignalMapper and any other solution presented uses it too then that doesn't matter.. I'd probably use QActionGroup anyway.

[high_flyer]

Do anyone know when this all changed and the case keyword accepted non-constant values?

Sorry - was typing faster then thinking.

You're doing it wrong. See the Open Closed Principle.

Your argumentation is conflicting.
First you said:

Every time they add a new shortcut/button/whatever to trigger a set_m event they would have to modify their original object to account for it.

This is true in any case, unless the connection are done in a loop, with some kind of parametrization.
In addition, by introducing such changes Open Close has been breached, unless, the new additions come in a new derived object, which again, would be "legal" to implement the slot for it.
And then - you say that due to Open Close no such changes should be made, if so, what ever the original implementation was, it should not be changed, be it manually connecting signals, or using SignalMapper.

Don't misunderstand, I am not saying that good design should not be used.
But what I suggested (very crudely - which wysota made much nicer) is that the use of a signal mapper is an implementation detail which can be done differently as well.
Again - for dynamically changing elements that need to be connected at runtime, I think a signal mapper is perfect.

Clean, reusable, object oriented.

Well, almost - in the sense that nrobert is talking about.
If you put the allocation of the QShorcut's in a for loop, and you feed the list of shortcuts from out side, then - it is Open Closes compliant, and in deed reusable while extendable, and if you add new objects all you have to do is implement the new slots for them. (in a derived class of course )

[nroberts]

Clean, reusable, object oriented. The only thing that hurts is the use of sender() but since QSignalMapper and any other solution presented uses it too then that doesn't matter.

It actually DOES matter, and it matters a great deal. Again you'll need two versions of the function, one that uses sender(), and the other that accepts the index to set. Use of the signal mapper does not impose this pointless dependency on there being a sender upon an object that shouldn't need it. It sticks the stink of that dependency into one place, a class that's sole responsibility is to bind values to senders and forward the signal.

It may seem like a pedantic issue to some but it's decisions just like this, about seemingly little things, that can make or break a project. First and foremost on any developer/designer's mind should be to NEVER create unnecessary dependencies. Your proposal does.

[wysota]

It actually DOES matter, and it matters a great deal. Again you'll need two versions of the function, one that uses sender(), and the other that accepts the index to set.

The code I presented is complete, there is just one slot and one function call.

Use of the signal mapper does not impose this pointless dependency on there being a sender upon an object that shouldn't need it. It sticks the stink of that dependency into one place, a class that's sole responsibility is to bind values to senders and forward the signal.

QSignalMapper uses sender() itself so that's just changing the point where it is used. Unfortunately the solution with signal mapper fails immediately when you insert a widget in the middle of the stacked widget. Using QList or QActionGroup doesn't contain this flaw. And it doesn't impose any additional dependencies since QSignalMapper also uses a container to store a list of mappings. My solution can easily be adapted to use a list instead of a map to avoid a problem when pages are dynamically added into the stack.
The fact where you do the encapsulation is meaningless.

It actually DOES matter, and it matters a great deal. Again you'll need two versions of the function, one that uses sender(), and the other that accepts the index to set.

The code I presented is complete, there is just one slot and one function call.

Use of the signal mapper does not impose this pointless dependency on there being a sender upon an object that shouldn't need it. It sticks the stink of that dependency into one place, a class that's sole responsibility is to bind values to senders and forward the signal.

QSignalMapper uses sender() itself so that's just changing the point where it is used. Unfortunately the solution with signal mapper fails immediately when you insert a widget in the middle of the stacked widget. Using QList or QActionGroup doesn't contain this flaw. And it doesn't impose any additional dependencies since QSignalMapper also uses a container to store a list of mappings. My solution can easily be adapted to use a list instead of a map to avoid a problem when pages are dynamically added into the stack.
The fact where you do the encapsulation is meaningless.

[nroberts]

Your argumentation is conflicting.

No, it isn't. You haven't understood it.

You quote me as saying, "Every time they add a new shortcut/button/whatever to trigger a set_m event they would have to modify their original object to account for it."

This is a statement was directed against your version. It is true that a violation of LSP is always also a violation of OCP but it can be easier to see it as LSP violation immediately. The "original object" here is that class that has the set_m() function.

This is true in any case, unless the connection are done in a loop, with some kind of parametrization.

No, this is not at all true in any case. Your suggestion uses particular button pointers (and lets just assume for a moment that this actually works) to refer to indexes and this is hard-coded into your version of set_m(). In order to connect a new button to your object you end up having to add additional cases within set_m(). This violates OCP.

In the original version the OP can attach as many signal generating objects as they want, and even call the function directly without any signal, without ever changing set_m() at all. All they have to do is attach an integral value (the index) with the signal that's being popped and link that to the set_m() function/slot. This is *extension* and is NOT a violation of OCP.

And then - you say that due to Open Close no such changes should be made, if so, what ever the original implementation was, it should not be changed, be it manually connecting signals, or using SignalMapper.

And as you can see, by using the SignalMapper no such change is made. The only changes made are in client code, which does little more than assemble a collection of objects, which as it should be. You want your changes to consist of nothing more than extension and reassembly.

Don't misunderstand, I am not saying that good design should not be used.
But what I suggested (very crudely - which wysota made much nicer) is that the use of a signal mapper is an implementation detail which can be done differently as well.

This is sort of true. Use of the SignalMapper is just an implementation detail. What it does and how it does it could be done a variety of different ways and the SetM object wouldn't care or know. Where to put that stuff though is not "just" an implementation detail for where both of you are proposing to place it imposes coupling that needn't and therefore shouldn't exist. Any amount of signal remapping methodologies could be applied in both this case and others. Good design dictates that you don't tie the SetM object to any one of them, nor its clients; you stick these details in separate objects that deal with this issue and this issue alone, deferring the details of what to do about the mapping that's created to other objects.

Again - for dynamically changing elements that need to be connected at runtime, I think a signal mapper is perfect.

It does exactly what it's made to do, and perhaps that's what the OP needs and perhaps it is not. What is certain that they don't want is to put the details of this decision in the object that set_m() is a part of.

The code I presented is complete, there is just one slot and one function call.

Your version is NOT complete in comparison to the OP's object. The original set_m() interface accepted an index parameter. It could therefor be called in a great variety of ways that neither of your methods can fully account for. The former fails whenever you want to connect to something besides a shortcut. The latter fails when you wish to connect the signal to an object that can generate the indexes itself. Both fail when you wish to simply pragmatically (ie, not in a signal) change the index.

Leaving the OP's object alone, as we should, and implementing the mapping magic in an external object such as SignalMapper, is exactly the thing that will both get the OP what they want and leave their class uncoupled to the implementation details of that mapping. It will allow them a greater variety of extensions in the future, all without having to change one single line of anything within the object they're attempting to control.

Whether or not QSignalMapper is the appropriate answer, it does seem to be in this case, is beside the point. The point is to NOT tie the set_m() object to anything in particular, allowing for the greatest amount of reuse and maintainability both for this object AND for the mapping functionality.

The fact where you do the encapsulation is meaningless.

I should hope you are blushing now. WHERE to do encapsulation is exactly the point of everything in object oriented and generic programming. If it was meaningless where to do encapsulation then encapsulation itself would be completely meaningless and redundant. The whole idea of OO would be absurd.

Yet another way to examine why both of you are in error is to look at the Single Responsibility Principle. This states that any object, function, or construct, should have a single and solitary purpose.

The OP has an object that governs something, and it doesn't matter what, that is indexed. That is all it should do. The set_m(int) function is related to this purpose because it changes the current index in that model. It does one thing and one thing only: it changes that current index. So far everything is the way it should be.

Now both of you are suggesting that the user also embed widget mapping into the things that the set_m() function does. We know that this isn't a correct responsibility for this function to have because the index of this model has been shown to us as not being particular UI elements. At the very least the mapping of widget to index should not be within the set_m() function, who's sole responsibility is to change the current index.

The question that remains at that point is whether or not the mapping function belongs in the object at all. Given the information we've been given by the OP we have no reason to believe that this is the case. If they had told us the intention of the object is to provide a model that can be indexed by UI element then the story would be different, but we haven't been told that. Even if we where we'd have to question the intent since this is a Newbie channel.

Truth is we have 3 clearly distinct types of responsibility here:

1) Respond to user input and generate signals.
2) Map signals/senders into an index
3) Manage an indexed whatever.

All three can be turned into generic abstractions for reuse in other scenarios and in fact, until you guys decided to couple 2&3, two of these things where already abstracted. The only one remaining was the mapper and there's at least one that exists and can do some things already.

Again, violation of the SRP is violation of OCP for when you tie 2&3 together then if you wish to change the behavior of the program you are no longer simply assembling a different collection of objects, you have to alter one object from the perspective of one or both of two directions. While keeping them separate allows you to simply replace THE part that needs to behave differently.

[hakermania]

Omg now I can understand the power of a simple question

[wysota]

Your version is NOT complete in comparison to the OP's object.

(...)

I should hope you are blushing now. WHERE to do encapsulation is exactly the point of everything in object oriented and generic programming.

The point is to have the encapsulation in the first place. And it is you who should be blushing if you suggest using an external signal mapper object as a good design. Read your own posts again and ask yourself whether an external signal mapper object is not a dependency here and is it the right encapsulation type and also think about all the other stuff you have written. It's nice that you know the rules but you are violating them yourself. If you want to use a signal mapper, it should be part of a solid black-box object only exposing means to register a shortcut to change the current index of a tab-widget (or whatever the component is) either through a has-a or a is-a relation. As for your first argument QTabWidget and QStackedWidget already contain a slot taking an int and changing the current index so there is no need in duplicating it and the slot I have shown should be a private one (ignoring for a moment that there are no such things as private slots) to make sure it only gets called in a valid situation by a valid object. And preguessing your next argument that the slot might be doing something more than just changing indexes I will say that if you want to do more then you should connect to a signal emitted when a current index is changed to be sure the behaviour stays consistent regardless of what causes the current index to change. If you want more complex semantics then it's still best to wrap it around the infrastructure for signalling index changes.

I don't (and neither does Daniel) say using a signal mapper is a bad approach here. I say that the sole fact of using a signal mapper doesn't really solve the problem especially since I pointed out a situation where it fails completely when used as an external object hence making the whole "architecture" completely unreliable and unreusable in an Open World. Sure, in some situations it will work, as will Dani's solution and a bunch of others so I don't see why we should value one over another. Moreover I say that if you wrap (i.e. subclass) the widget container class to establish a separate (internal) list (or map) of dynamically maintained mappings, this will be the ultimate solution to the problem. In the end you don't really care whether some shortcut maps to "1" or "7", you only care that a particular widget pops up so this is what the interface should expose, the numbers are just a technical detail.

[squidge]

void MyClass::mySmartSlot()
{
   switch(sender())
   {
     case pButton1: //do button stuff;
[...]

void MyClass::mySmartSlot()
{
   switch(sender())
   {
     case pButton1: //do button stuff;
[...]

To copy to clipboard, switch view to plain text mode 

When I first learnt about the switch...case statement many years ago, you could only use constant values in case statements, so if you wanted to compare pointer values as above, you had no choice but to use 'if...else' statements, so I've automatically used QSignalMapper in all of the above cases where I have had similar requirements.

Do anyone know when this all changed and the case keyword accepted non-constant values?

[wysota]

Do anyone know when this all changed and the case keyword accepted non-constant values?

Does it really do that?

[wysota]

Using QActionGroup is also an option here. Sooner or later you'll probably want to assemble a menu with the options anyway.