> We try to sort the includes.
OK
> #if QT_VERSION >= 0x040700
> > + search_->setPlaceholderText(tr("Search"));
> #endif
Nice.
> if (!item->child(i)->isDisabled()) {
> (although now I see this is introduced in Qt 4.3)
I was using other flags before the final patch, isDisabled is better.
> > - switchPanel( item->child(0), previous );
> > + switchPanel( item->child(i), previous );
>
> Style: no spaces next to the braces.
Mmmmh, spaces were already there. However I'll remove them.
> It's strange that Qt doesn't paint the items in the Disabled
> color whenever the item is disabled.
I agree.
> "this" is unnecessary.
More clear IMHO. However I'll remove it.
> Usually we use iterators like:
>
> PanelMap::const_iterator it = panel_map_.begin();
> PanelMap::const_iterator end = panel_map_.end();
> for (; it != end; ++it) {
> QTreeWidgetItem * item = *it;
> [..]
> }
I think this syntax it's very hard to understand, we should use Qt's foreach
macro (if we'll ever want to drop Qt it's just a file to import), much more
elegant. However if it's a strict coding rule I'll follow it.
About the "if else if else if..." question:
> >> This is really ugly, but I can't think of how to solve it right now.
I know, see my comment. I asked on #Qt [1] and this seems to be the best
solution. If we want something more clean and OO we should subclass each widget
we want to be searchable, but it's not a good solution IMHO. It would be
interesting to have somthing in QWidget like QWidget::ToString() or even
QObject::ToString(), like in .Net.
> > Would using typeid help a little bit?
qobject_cast is the best solution AFAIK, because I'm considering, for instance,
QAbstractButton and not QPushButton or QCheckBox (which inherit
QAbstractButton). Then, if we want to use reflection we should check the whole
type hierarchy, which is exactly what qobject_cast does.
In the next days I'll post a fixed patch.
- To translate the string I used tr("Search"), is that correct?
- Currently the widgets are highlighted in red, alternative ideas? Maybe bold or
nothing at all.
- Do you think a "rubber" button in (or next to) the QLineEdit is necessary to
clean the search box?
I should also add something to reset the search box when closing the dialog.
Apart from these (useful!) comments, does the patch work?
venom00
P.S. Thanks for reviewing my patch! :)
[1] irc://irc.freenode.org/Qt
