setModelData crashes program

JonB

@gabor53 said in setModelData crashes program:

How to check the content of whatComboEditor?
** just to mark the line that crashes

OMG, you put a literal ** in a legal C++ code place on the faulty line, not in a comment, where I was supposed to understand it was a comment....... That did not occur to me.

So as the others have said, where you go qobject_cast<QComboBox*>(editor) your editor parameter is either NULL, does not point to a QComboBox, or points to what was a QComboBox but is now invalid (e.g. disposed). Can't you just look at it in a debugger?

mrjj

Hi
Just a small question.
Is this code related to
https://forum.qt.io/topic/83393/centering-qcombobox-in-table-cell
where you also used a layout to center the combobox ?
I wonder if the
QComboBox* whatComboEditor = qobject_cast<QComboBox*>(editor);
then is correct ?
but add check to it
if (!whatComboEditor) {
qDebug() << "fail to cast whatComboEditor";
return;
}

gabor53

@mrjj
It is related to the referenced post. This is part of the same delegate. I added the suggested check and I got the "fail to cast whatComboEditor" message.
Here is the whole delegate:

#include "whatfixviewdelegate.h"

WhatFixViewDelegate::WhatFixViewDelegate(QObject* parent) : QStyledItemDelegate(parent) {

}

QWidget* WhatFixViewDelegate::createEditor(QWidget* parent, const QStyleOptionViewItem& option, const QModelIndex& index) const {

  Q_UNUSED(option);
  Q_UNUSED(index);

  QWidget* w = new QWidget( parent );

  QComboBox* whatComboEditor = new QComboBox(w);
  whatComboEditor->setFrame(true);
  whatComboEditor->setStyleSheet("background-color:rgb(255,217,229);" );
  whatComboEditor->setEditable(true);

  whatComboEditor->setMaximumWidth (100);
  whatComboEditor->setMinimumWidth (100);
  whatComboEditor->setMaximumHeight (30);
  whatComboEditor->setMinimumHeight (30);

  whatComboEditor->setSizePolicy(QSizePolicy::Expanding, QSizePolicy::Expanding);

  QVBoxLayout* whatLayout = new QVBoxLayout;
  whatLayout->addWidget (whatComboEditor, 0, Qt::AlignCenter);
  whatLayout->setMargin (2);

  w->setLayout (whatLayout);

  QSqlQuery queryLoadWhat("SELECT What FROM  What_Table ORDER BY What asc", db);

  while(queryLoadWhat.next()) {
    QString whatItem = queryLoadWhat.value(0).toString();
    whatComboEditor->addItem(whatItem);
  }
  return w;
}

void WhatFixViewDelegate::setEditorData(QWidget* editor, const QModelIndex& index) const {

  QComboBox* whatComboEditor = qobject_cast<QComboBox*>(editor);
  if(whatComboEditor) {
    whatComboEditor->addItem(index.model()->data(index, Qt::EditRole).toString());
    whatComboEditor->setCurrentText(index.model()->data(index, Qt::EditRole).toString());
  }
}

void WhatFixViewDelegate::updateEditorGeometry(QWidget* editor, const QStyleOptionViewItem& option, const QModelIndex& index) const {
  Q_UNUSED(index);

  editor->setGeometry(option.rect);
}

void WhatFixViewDelegate::setModelData(QWidget* editor, QAbstractItemModel* fixModel, const QModelIndex& index) const {
  QComboBox* whatComboEditor = qobject_cast<QComboBox*>(editor);
  if (!whatComboEditor) {
    qDebug() << "fail to cast whatComboEditor";
    return;
  }



  QString newWhatText;

  newWhatText = whatComboEditor->currentText ();//crashes
  qDebug() << "newWhatText: " <<  whatComboEditor->currentText ();

  fixModel->setData (index, newWhatText, Qt::EditRole);

  fixModel->submit ();
}

What should be the next step? Thank you.

mrjj

Hi
You should find the combobox in the widget with
QComboBox* whatComboEditor = editor->findChild< QComboBox*>();
if (whatComboEditor ) { ...use it... }
in the other functions since you now have a real widget in between.
as you say

QWidget* WhatFixViewDelegate::createEditor(QWidget* parent, const QStyleOptionViewItem& option, const QModelIndex& index) const {
  QWidget* w = new QWidget( parent );
....
  return w; <<< that is the place holder widget you return as "editor"
}

SGaist

In your case, you should call findChild on editor. Your QComboBox is in a layout inside a QWidget so you can't cast it like that directly.

An alternative would be to create a custom widget which wraps the QComboBox and that you could use in the same manner.

JonB

And always check the return result of a qobject_cast<>, for safety, certainly while developing! :)

gabor53

Thank you. I redid it as recommended and now it looks like this:

#include "whatfixviewdelegate.h"

WhatFixViewDelegate::WhatFixViewDelegate(QObject* parent) : QStyledItemDelegate(parent) {

}

QWidget* WhatFixViewDelegate::createEditor(QWidget* parent, const QStyleOptionViewItem& option, const QModelIndex& index) const {

  Q_UNUSED(option);
  Q_UNUSED(index);

  QWidget* w = new QWidget( parent );

  QComboBox* whatComboEditor = new QComboBox(w);
  whatComboEditor->setFrame(true);
  whatComboEditor->setStyleSheet("background-color:rgb(255,217,229);" );
  whatComboEditor->setEditable(true);

  whatComboEditor->setMaximumWidth (100);
  whatComboEditor->setMinimumWidth (100);
  whatComboEditor->setMaximumHeight (30);
  whatComboEditor->setMinimumHeight (30);

  whatComboEditor->setSizePolicy(QSizePolicy::Expanding, QSizePolicy::Expanding);

  QVBoxLayout* whatLayout = new QVBoxLayout;
  whatLayout->addWidget (whatComboEditor, 0, Qt::AlignCenter);
  whatLayout->setMargin (2);

  w->setLayout (whatLayout);

  QSqlQuery queryLoadWhat("SELECT What FROM  What_Table ORDER BY What asc", db);

  while(queryLoadWhat.next()) {
    QString whatItem = queryLoadWhat.value(0).toString();
    whatComboEditor->addItem(whatItem);
  }
  return w;
}

void WhatFixViewDelegate::setEditorData(QWidget* editor, const QModelIndex& index) const {
  QComboBox* whatComboEditor = editor->findChild<QComboBox*>();

  if(whatComboEditor) {
    whatComboEditor->addItem(index.model()->data(index, Qt::EditRole).toString());
    whatComboEditor->setCurrentText(index.model()->data(index, Qt::EditRole).toString());
  }
}

void WhatFixViewDelegate::updateEditorGeometry(QWidget* editor, const QStyleOptionViewItem& option, const QModelIndex& index) const {
  Q_UNUSED(index);

  editor->setGeometry(option.rect);
}

void WhatFixViewDelegate::setModelData(QWidget* editor, QAbstractItemModel* fixModel, const QModelIndex& index) const {

  QComboBox* whatComboEditor = editor->findChild<QComboBox*>();
  if (whatComboEditor) {
      QString newWhatText;
    
      newWhatText = whatComboEditor->currentText ();//crashes
      qDebug() << "newWhatText: " <<  whatComboEditor->currentText ();
    
      fixModel->setData (index, newWhatText, Qt::EditRole);
    
      fixModel->submit ();
  }
}

Now it doesn't crash, but when I add an item from the QComboBox, all the data from the line disappears like this:

When I reload the table all the data are back and the newly chosen item is also there (it was saved in the db). What can I add to prevent the data from disappearing?

VRonin

@gabor53 said in setModelData crashes program:

What can I add to prevent the data from disappearing?

Sounds like a model bug. What is fixModel?

JonB

@JNBarchan said in setModelData crashes program:

And always check the return result of a qobject_cast<>, for safety, certainly while developing! :)

In my nice (non-Qt) C# background, this kind of casting is built into the language. For qobject_cast<> we write:

ComboBox cb = editor as CombBox;

where the as operator returns null if editor cannot be cast to ComboBox at runtime. We use this if editor might not be a ComboBox.

However, if editor must be/is intended to be a ComboBox, we write:

ComboBox cb = (ComboBox) editor;

where the direct cast throws a runtime Exception if editor is not a ComboBox (i.e. as would return null). This saves a lot of code/accidents.

It would be nice if Qt offered a "variant" of qobject_cast<> in C++ which checked and automatically threw an Exception if the return result is NULL/0....?

VRonin

you just need to add Q_ASSERT(whatComboEditor); to have that

JonB

@VRonin said in setModelData crashes program:

you just need to add Q_ASSERT(whatComboEditor); to have that

Ah, OK. It's just that you have to remember to write that every time after each qobject_cast<>, it would be nice to have them combined in one statement. I suppose you would write:

Q_ASSERT(whatComboEditor = qobject_cast<>)

but is Q_ASSERT code removed in non-debug code and/or does it evaluate its argument more than once (assuming it's a macro)?

I would like a qobject_assert_cast<> :)

EDIT: Yes, I see you cannot use Q_ASSERT(whatComboEditor = qobject_cast<>) in non-debug as the code gets removed.

SGaist

That's not specific to Q_ASSERT, it's the same for the standard C++ assert. Always test the result of a function and not the function itself.

VRonin

If you want to use it in code that should still be present in release mode you can use Q_ASSUME

JonB

@VRonin said in setModelData crashes program:

If you want to use it in code that should still be present in release mode you can use Q_ASSUME

Um, ~~I don't think so~~ are you sure: http://doc.qt.io/qt-5/qtglobal.html#Q_ASSUME states it's a compile-time hint, and you get "undefined behaviour" if the condition is false. We need a runtime check of the return result of qobject_cast<>.

EDIT: Unless the docs are misleading, and the true implementation is via https://git.merproject.org/faenil/qtbase/commit/3b0ed624351441a2d7be45cf9582fd36955ae860 , which uses Q_ASSERT_X?

VRonin

Q_ASSUME in debug mode is like Q_ASSERT in release mode, if whatComboEditor is null the program would crash anyway so you can't do much worse.

The undefined behaviour comes from when you use the return value: if(Q_ASSUME(false)) is undefined behaviour. if it just wraps an operation you do not check anyway, Q_ASSUME does nothing to your compiled binary in release mode

JonB

@VRonin
That wasn't quite the point, though. I was talking about writing the single-line statement (if Qt won't offer a qboject_assert_cast<>):

Q_ASSERT(whatComboEditor = qobject_cast<>)

versus

Q_ASSUME(whatComboEditor = qobject_cast<>)

The point being that it does the assignment. So we do need the expression to evaluate, just we don't need/care to do the "assertion" part of testing the result in release build. This would not work for Q_ASSERT as (I believe) it's a macro which produces nothing (no code at all) in release build. That was when you suggested Q_ASSUME.

VRonin

Q_ASSERT(statement);
- Debug: if(!statement) triggerAnAssertion();
- Release: ;
Q_ASSUME(statement);
- Debug: if(!statement) triggerAnAssertion();
- Release: statement;

Is the difference clearer now?

gabor53

@VRonin
I added

 Q_ASSERT(whatComboEditor);

but it behaves the same.

VRonin

@VRonin said in setModelData crashes program:

Sounds like a model bug. What is fixModel?

gabor53

@VRonin

QSqlTableModel* fixModel = new QSqlTableModel(this);
  fixModel->setTable ("Items");
  fixModel->setEditStrategy (QSqlTableModel::OnRowChange);
  fixModel->setSort (2, Qt::DescendingOrder);

  fixModel->select ();