[PATCH 1/2] BookmarkModel::insert uses a more low level approach to data insertion.

[Marcelo Lira]

It now uses a SQL query instead of a QSqlRecord. The reason for
using the former is that insertion through QSqlRecord increases the
row count, and emits a signal advertising it, before the row is
commited to the database. This inconsistent state is causing trouble
with the pin/unpin button in the mobile UI.

Reviewed-by: ?
---
src/core/BookmarkModel.cpp | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/src/core/BookmarkModel.cpp b/src/core/BookmarkModel.cpp
index 9726902..cef0c36 100644
--- a/src/core/BookmarkModel.cpp
+++ b/src/core/BookmarkModel.cpp
@@ -55,14 +55,19 @@ bool BookmarkModel::select()

void BookmarkModel::insert(const QString& name, const QString& url)
{
- if (contains(url))
+ if (url.isEmpty() || contains(url))
return;
- QSqlRecord record = this->record();
- record.setValue(QLatin1String("name"), name);
- record.setValue(QLatin1String("url"), url);
- record.setValue(QLatin1String("dateAdded"), QDateTime::currentDateTime().toTime_t());
-
- insertRecord(-1, record);
+ QModelIndex index = QModelIndex();
+ beginInsertRows(index, rowCount(index), rowCount(index));
+ QSqlQuery sqlQuery(database());
+ static QString insertStatement = QLatin1String("INSERT INTO bookmarks (name, url, dateAdded) VALUES (?, ?, ?)");
+ sqlQuery.prepare(insertStatement);
+ sqlQuery.addBindValue(name);
+ sqlQuery.addBindValue(url);
+ sqlQuery.addBindValue(QDateTime::currentDateTime().toTime_t());
+ sqlQuery.exec();
+ select();
+ endInsertRows();
submitAll();
}

--
1.7.9.5

[Marcelo Lira]

Reviewed-by: ?
---
src/mobile/qml/OverlayBar.qml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mobile/qml/OverlayBar.qml b/src/mobile/qml/OverlayBar.qml
index 8482f3f..9148c4e 100644
--- a/src/mobile/qml/OverlayBar.qml
+++ b/src/mobile/qml/OverlayBar.qml
@@ -73,8 +73,8 @@ Item {
pressedImage: pin ? "qrc:///mobile/overlaybar/btn_pin_unpressed" : "qrc:///mobile/overlaybar/btn_pin_pressed"
unpressedImage: pin ? "qrc:///mobile/overlaybar/btn_pin_pressed" : "qrc:///mobile/overlaybar/btn_pin_unpressed"
onClicked: {
- overlayBar.pinToggled();
pin = !pin;
+ overlayBar.pinToggled();
}
}

--
1.7.9.5

[Rafael Brandao]

Hm, how weird. Why we don't use the signal pinChanged instead? It looks wrong you're changing a value and explicitly emitting its signal.

--
Rafael Brandao @ INdT

[Rafael Brandao]

On Fri, Jun 15, 2012 at 6:50 PM, Marcelo Lira <marcel...@openbossa.org> wrote:

Any reason why select() come right after exec()? Shoulnd't endInsertRows come first?

 

    submitAll();
 }

--
1.7.9.5

[Marcelo Lira]

I agree that's a bit weird, and thought of taking it out, but strictly
speaking the "pinToggled" signal tells that the pin was toggled. If an
external entity (the BookmarksModel, for example) disagrees, it may
adjust the pin state.

Now, about using the onPinChanged signal.
This is the code that listens to the pin toggling:

OverlayBar {
onPinToggled: BookmarkModel.togglePin(visibleTab.url);
Connections {
target: BookmarkModel
onCountChanged: {
if (visibleTab)
overlayBar.pin = BookmarkModel.contains(visibleTab.url);
}
}
}

The UI is pinned/unpinned, the change is propagated to the model, and
when the model changes (if everything works fine there) the changed is
confirmed in the UI.
Using onPinChanged would result in an infinite loop, and an scheme to
go around that would be as complicated as using onPinToggled.

--
Marcelo Lira dos Santos
INdT - Instituto Nokia de Tecnologia

[Rafael Brandao]

Ugh, this code there is very confusing, I'm trying to think into a solution for it but I'm just very tired right now. :-(

[Marcelo Lira]

At first it was for copy and paste reasons, but I noticed that when
select() comes before exec() the row count at the moment when the
BookmarkModel.onCountChanged() signal is emited, the count is -1 the
real number of rows.

[Marcelo Lira]

Then accept, for the time being. :)

On Fri, Jun 15, 2012 at 7:31 PM, Rafael Brandao

[Rafael Brandao]

I mean maybe you should use:

exec()

endInsertRows()

and then select();

Doesnt seem more logical that we finish inserting rows after exec()?

[Rafael Brandao]

Ok then, but we should fix it at some point.

(it's not like this is more urgent than our performance regressions) :-P

r=me

[Marcelo Lira]

On Fri, Jun 15, 2012 at 7:39 PM, Rafael Brandao
<rafae...@openbossa.org> wrote:
> I mean maybe you should use:
>
> exec()
> endInsertRows()
> and then select();
>
> Doesnt seem more logical that we finish inserting rows after exec()?

Logical? You must be new here. :D
I agree, but doing that also causes the row count during
rowCountChanged emission to be 1 behind the real number of rows.