2

I am using Qt5 on Windows 7. In my current app I have the following piece of code that changes the background color of some push-buttons:

...
for(int i = 0; i < layout()->count(); i++)
{
    QPushButton * button = static_cast<QPushButton*>(layout()->itemAt(i)->widget());
    button->setStyleSheet(backgroundColor);
}

Well, I have 2 questions about the above code:

  1. Is it ok/correct to use static_cast or should I use some other type of casting?

  2. Is it possible to use foreach instead of the for loop above?

halfer
  • 19,824
  • 17
  • 99
  • 186
סטנלי גרונן
  • 2,917
  • 23
  • 46
  • 68

3 Answers3

13

You should use qobject_cast so you can check if the cast was successful. It returns 0 if the cast failed.

QPushButton * button = qobject_cast<QPushButton*>(layout()->itemAt(i)->widget());
if(button)
    // cast ok
else
    // cast failed

You can't use foreach as you would need a container for that.

thuga
  • 12,601
  • 42
  • 52
2

It is technically acceptable to use static_cast only if you're sure that the layout only contains widget items and they all contain a QPushButton. Since this is error prone in face of code modifications, I don't suggest doing it.

Instead, it is desirable to use range-for in C++11 or foreach/Q_FOREACH by using a layout iterator adapter. The iterator adapter also solves the problem of iterating only the elements of a type you desire and makes your code safe in face of modifications.

Your can then use range-for and this code is safe even if no QPushButtons are in the layout, and will cope with any kind of layout item gracefully by ignoring it as it should:

for (auto button : IterableLayoutAdapter<QPushButton>(layout())) 
  button->setStyleSheet(backgroundColor);
Community
  • 1
  • 1
Kuba hasn't forgotten Monica
  • 95,931
  • 16
  • 151
  • 313
  • Thx for your answer. I followed the link you provided above. You said something like using `foreach` in C++11 is a bad thing. Why is that? – סטנלי גרונן Jun 22 '16 at 15:39
  • @groenhen Because it's a butt-ugly macro that can sometimes bite you, and you've got range-for for a reason: so that you'd use it! – Kuba hasn't forgotten Monica Jun 22 '16 at 16:02
  • Geee, it seems `foreach` is something Qt-specific. So, I guess your aversion on it is because it is kinda polluting C++ code... OTOH, I was just wondering if `range-for` has any advantage over simple `for` loop, I mean something like smaller code and/or faster code?... – סטנלי גרונן Jun 22 '16 at 16:52
  • 2
    @groenhen Range-for clearly expresses your intent and is subject to a number of optimizations that might fail otherwise. In a `for` loop, you have an induction variable or an iterator - good if you need them, but if your intent is "give me all elements (perhaps fulfilling a criterion) from a collection", then the induction variable/iterator is a distracting implementation detail and range-for is **the** direct way of writing it. `foreach` was a necessary hack in pre-C++11 days. 5 years into C++11's reign there's no point to it, except if your code needs to be compiled using obsolete tools. – Kuba hasn't forgotten Monica Jun 22 '16 at 17:36
1

If you are sure that all widgets are QPushButtons, then yes, static_cast is the best option (most efficient) Regarding the foreach, I'm not sure you can get the QLayoutItems as some standard container, so I'm not sure you can do it.

gbehar
  • 1,229
  • 10
  • 10