Fixed executing current SQL line not working for multiple lines SQLs

[revolter]

This PR selects the text from the last semicolon (or the start) found before the cursor and up to the first semicolon (or the end) found after the cursor and executes it.

Examples:

SELECT *
FROM table| a; SELECT *
FROM table b

runs the first query.

SELECT *
FROM table a; |SELECT *
FROM table b

runs the second query.

SELECT *|
FROM table a; SELECT *
FROM table b

runs the first query.

SELECT *
FROM table a; SELE|CT *
FROM table b

runs the second query.

[justinclift]

@innermous @MKleusberg Would either of you have time to review/test this?

My head is in configuration management learning at the moment, not C++, so I won't have time to look at it in the next day or two. 😦

[revolter]

And I would like for @chrisjlocke to test this, if you can, since you asked for the "execute current line" fix and it looks like you use it more often than I do (which is not at all 😆)

[chrisjlocke]

No problem. I use it a lot as when creating tables, its handy (in the same tab, so you can see the fields) to create indexes, etc. Then add a test entry. Then go back and tweak that text field to hold bananas. Etc.

[vtronko]

Consider this. No allocations, all in-place.

diff --git a/src/MainWindow.cpp b/src/MainWindow.cpp
index 8c30e1c..cdbb4a5 100644
--- a/src/MainWindow.cpp
+++ b/src/MainWindow.cpp
@@ -865,13 +865,10 @@ void MainWindow::executeQuery()
         int position = editor->positionFromLineIndex(cursor_line, cursor_index);

         QString entireSQL = editor->text();
-        QString firstPartEntireSQL = entireSQL.left(position);
-        QString secondPartEntireSQL = entireSQL.right(entireSQL.length() - position);
+        int begin = entireSQL.lastIndexOf(';', -(entireSQL.size() - position + 1)) + 1;
+        int end = entireSQL.indexOf(';', position);

-        QString firstPartSQL = firstPartEntireSQL.split(";").last();
-        QString lastPartSQL = secondPartEntireSQL.split(";").first();
-
-        query = firstPartSQL + lastPartSQL;
+        query = entireSQL.mid(begin, end - begin);
     } else {
         // if a part of the query is selected, we will only execute this part
         query = sqlWidget->getSelectedSql();

[vtronko]

Look at my suggestion, or at least use QStringRefs.

[revolter]

It's a case of readability vs conciseness, and I find -(entireSQL.size() - position + 1)) + 1 not clear at all. Having variables for this kind of calculations has the advantage of clearly stating what it does in the actual name of the variable. Another useful thing is that it's a lot easier to debug when stepping through it.

Aren't these kinds of things either: 1) not performance costly and/or 2) quite efficiently optimized out by the compiler?

Having someone new come across this code, he might not understand what it does, and sometimes, the fact that a comment is needed for some code should raise some questions about the clarity of it. And this feels like premature optimization too.

It looks like:

We suggest that you only use this class (QStringRef) in stable code where profiling has clearly identified that performance improvements can be made by replacing standard string operations with the optimized substring handling provided by this class.

so I'm better off to profile it before starting to optimize it.

[justinclift]

Interesting. Both ways seem ok to me... personally speaking, in this particular case the version with begin/end/query variables looks like it would be clearer. With some thoughts: 😄

• The -(entireSQL.size() - position + 1) fragment would probably be clearer assigned to a useful variable name.
• Adding some comment(s) to the code, explaining what these lines of code are doing would be useful too

[vtronko]

Use stringrefs then. There's no need in QString's down here.

[revolter]

@innermous, Could you please change it to use QStringRefs? The Allow edits from maintainers is on for this PR 😄

[vtronko]

You guys don't value the performance. Two indexOf operators vs split everything for god's sake.

[revolter]

Ok, you're right, rewrite it to use indexOf

[vtronko]

God, this Qt4 is awesome.

[vtronko]

Sorry for inconveniences. I should've known that split is not existing for QStringRef's in Qt4.

[revolter]

Can you fix the Qt4 build? Also, you can change it to use indexOf if you want.

[vtronko]

@revolter I don't know why it failed. Merged commit doesn't have any QStringRef's.
It will probably be alright with next commit pushed.
Upd: I tried "Reload job", maybe it helps.

[revolter]

Actually, the merged code contains

QString firstPartEntireSQL = entireSQL.leftRef(position);

so neither

QString firstPartEntireSQL = entireSQL.left(position);

nor

QStringRef firstPartEntireSQL = entireSQL.leftRef(position);

This is the Travis error:

/home/travis/build/sqlitebrowser/sqlitebrowser/src/MainWindow.cpp:868:55: error: conversion from ‘QStringRef’ to non-scalar type ‘QString’ requested
QString firstPartEntireSQL = entireSQL.leftRef(position);

[vtronko]

@revolter my god... I screwed up... I tried to fall back to your first diff, but forgot about leftRefs...
Can you send another commit with changes, straight push? I am away from PC and can't fix the mess I've done.

[revolter]

No problem, I'll try to push them straight from the website.

[justinclift]

@chrisjlocke When you get a chance, would you be ok to verify this works now? 😄

[chrisjlocke]

I certainly will once the Windows' builds are back up and running, which I understand from your reply in another topic/thread/issue/doodah is tomorrow. :)

[chrisjlocke]

Windows builds are back up and running. so just had a look at this. Works fine, but the obvious niggle is that this doesn't work at all:

CREATE TABLE test3 (id TEXT PRIMARY KEY ASC);

CREATE TABLE test4 (id TEXT PRIMARY KEY ASC);|

The second statement isn't run, and no error is displayed. Obviously its not a showstopper, but 'unexpected', if you know what I mean. Just move the cursor back a character, and it'll run that line...

CREATE TABLE test3 (id TEXT PRIMARY KEY ASC);

CREATE TABLE test4 (id TEXT PRIMARY KEY ASC)|;

but if you don't know that, you'd wonder why it isn't working.
From your logic, I guess the cursor has to be before a ;. From my point of view, a 'line' is a line of text. ie, 'execute this line of SQL where the cursor is'. I can see that view won't work if you're expecting SQL statements to be split across lines (as in your example).

Can't think of an easy workaround. Initially I could, but its big bloaty code... DB4S would be a 300 MB download...

[revolter]

Well, I think it's pretty expected, as you don't have the cursor inside any SQL. It can't do both: run the current text line and run the current SQL.

From my point of view, a 'line' is a line of text. ie, 'execute this line of SQL where the cursor is'.

Well, here you're contradicting yourself. Previously, you expected it to run the current SQL and not the "line of text", as having an SQL split on multiple lines was causing problems.

[chrisjlocke]

No, I've never joined multiple SQL statements on the same line with a ; to join them, but have split SQL across multiple lines, so

 create table blah (
 id text primary key,
 field2 text);

So yes, guess I am contradicting myself. Sorry for the obvious confusion.
As stated, its not a problem, and the solution you've provided is more than adequate. :)

[justinclift]

From an end users's perspective, @chrisjlocke's first point is correct. It's extremely common to add ; to the end of a statement, then expect "Execute current line" to work.

Having to manually place the cursor before the ; won't be practical. Let's figure out a solution to make this work. 😄

Maybe we should check the position of the cursor before we run determine the query string? Then depending on the result of the position check, we figure out which part of the line to start working with.

[revolter]

Well, I could detect if the right part from the cursor is empty (after trimming it) and select the previous SQL before the semicolon.

[justinclift]

That sounds workable. 😄

[justinclift]

Hmmm, I can't see this getting into 3.9.1 as-is.

@revolter What are the chances of getting the cursor detection position thing today? 😄

[revolter]

Why not? I think 0.0001% of people will put the cursor after the last semi-colon and start hating us for not working. It improves a lot the functionality that was in 3.8.0.

I think I could start a VM and build the project there.

[justinclift]

It's going to be a lot more common than 0.001% of people. 😉

A very common pattern of use is to cut-n-paste a SQL command (with ; on the end) into the Execute SQL window, then run it. As this issue stands now, that no longer works with the execute current line button.

Just tested it here to be sure, and yeah... the execute current line button just "seems broken" from an end user perspective. eg pressing it does nothing

The cursor position thing will fix that, so it works "properly" from an end user's point of view. 😄

[revolter]

Why wouldn't you cut-n-paste a SQL and then run it with the Execute action instead of Execute current line action?

[justinclift]

Either way should work.

Having one just "not work" for no obvious reason is just not going to fly. 😉

It sounds like a fix for this by today isn't really working for you? If we delay 3.9.1 release until tomorrow would that be better? Note - It's not a bad idea anyway, as it will give some more time for translators to get things happening too.

[revolter]

Yeah, but previously, it was executing the wrong SQL completely.

If I manage to build the project I can fix it. And if not, I'll try to fix it blindly and send you a patch to build. But I was just trying to understand why are you seeing this corner case so important. I'll try to get it done tonight.

[justinclift]

Thanks @revolter. 😄

[revolter]

@chrisjlocke, I fixed the problem with the last character being a semicolon in 45affc9.

[justinclift]

Found another big bug in this while testing today. 😦

When the cursor is to the right of the ; symbol, but there are more lines further down... the next line gets executed instead of the current line.

For example:

With the cursor to the right of the ; of the 2nd SQL statement, the 3rd statement gets executed. That'll be an unexpected surprise for users.

[revolter]

@justinclift, Fixed in 531eddb (hopefully the last commit that touches that code 😆)

[justinclift]

Awesome, I'll verify it here, then pull it into 3.9.1. 😄

[justinclift]

Looks good here @revolter, well done. 😄

Added to 3.9.1. 🎉