tools/qmlformat: Improve formatting quite a bit

- Fix strings not getting properly escaped
- Fix if/else blocks not getting formatted correctly in some cases
- Remove unnecessary newlines
- Improve comment attachment

Change-Id: Id8264c069315b3c6bd9ba8e8ac991dbdce8581a7
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

3 files changed, 72 insertions, 18 deletions

diff --git a/tools/qmlformat/commentastvisitor.cpp b/tools/qmlformat/commentastvisitor.cpp
index 6d2bf49ddc..8264a6099e 100644
--- a/tools/qmlformat/commentastvisitor.cpp
+++ b/tools/qmlformat/commentastvisitor.cpp
@@ -198,13 +198,13 @@ void CommentAstVisitor::endVisit(StatementList *node)
 
 bool CommentAstVisitor::visit(UiObjectBinding *node)
 {
-    attachComment(node);
+    attachComment(node, Comment::Front | Comment::Back);
     return true;
 }
 
 bool CommentAstVisitor::visit(UiObjectDefinition *node)
 {
-    attachComment(node);
+    attachComment(node, Comment::Front | Comment::Back);
     return true;
 }
 
diff --git a/tools/qmlformat/dumpastvisitor.cpp b/tools/qmlformat/dumpastvisitor.cpp
index 09bb45c2bb..1ab09e463b 100644
--- a/tools/qmlformat/dumpastvisitor.cpp
+++ b/tools/qmlformat/dumpastvisitor.cpp
@@ -34,6 +34,9 @@ DumpAstVisitor::DumpAstVisitor(Node *rootNode, CommentAstVisitor *comment): m_co
     m_result += getOrphanedComments(nullptr);
 
     rootNode->accept(this);
+
+    // We need to get rid of one new-line so our output doesn't append an empty line
+    m_result.chop(1);
 }
 
 static QString parseUiQualifiedId(UiQualifiedId *id)
@@ -223,6 +226,17 @@ QString DumpAstVisitor::parsePatternElement(PatternElement *element, bool scope)
     }
 }
 
+QString escapeString(QString string)
+{
+    // Escape \r, \n and \t
+    string = string.replace("\r", "\\r").replace("\n", "\\n").replace("\t", "\\t");
+
+    // Escape "
+    string = string.replace("\"", "\\\"");
+
+    return "\"" + string + "\"";
+}
+
 QString DumpAstVisitor::parsePatternElementList(PatternElementList *list)
 {
     QString result = "";
@@ -301,7 +315,7 @@ QString DumpAstVisitor::parseExpression(ExpressionNode *expression)
     case Node::Kind_NumericLiteral:
         return QString::number(cast<NumericLiteral *>(expression)->value);
     case Node::Kind_StringLiteral:
-        return "\""+cast<StringLiteral *>(expression)->value+"\"";
+        return escapeString(cast<StringLiteral *>(expression)->value.toString());
     case Node::Kind_BinaryExpression: {
         auto *binExpr = expression->binaryExpressionCast();
         return parseExpression(binExpr->left) + " " + operatorToString(binExpr->op)
@@ -339,7 +353,7 @@ QString DumpAstVisitor::parseExpression(ExpressionNode *expression)
         QString result = "";
 
         result += parseExpression(condExpr->expression) + " ? ";
-        result += parseExpression(condExpr->ok) + ": ";
+        result += parseExpression(condExpr->ok) + " : ";
         result += parseExpression(condExpr->ko);
 
         return result;
@@ -474,21 +488,45 @@ QString DumpAstVisitor::parseStatement(Statement *statement, bool blockHasNext,
 
         m_blockNeededBraces = false;
 
-        QString if_false = parseStatement(ifStatement->ko, false, true);
-        QString if_true = parseStatement(ifStatement->ok, !if_false.isEmpty(), true);
+        QString ifFalse = parseStatement(ifStatement->ko, false, true);
+        QString ifTrue = parseStatement(ifStatement->ok, !ifFalse.isEmpty(), true);
+
+        bool ifTrueBlock = ifStatement->ok->kind == Node::Kind_Block;
+        bool ifFalseBlock = ifStatement->ko
+                ? (ifStatement->ko->kind == Node::Kind_Block || ifStatement->ko->kind == Node::Kind_IfStatement)
+                : false;
 
         if (m_blockNeededBraces) {
-            if_false = parseStatement(ifStatement->ko, false, false);
-            if_true = parseStatement(ifStatement->ok, !if_false.isEmpty(), false);
+            ifFalse = parseStatement(ifStatement->ko, false, false);
+            ifTrue = parseStatement(ifStatement->ok, !ifFalse.isEmpty(), false);
         }
 
-        QString result = "if (" + parseExpression(ifStatement->expression) + ") ";
+        QString result = "if (" + parseExpression(ifStatement->expression) + ")";
 
-        result += if_true;
+        if (ifTrueBlock) {
+            result += " " + ifTrue;
+        } else {
+            result += "\n";
+            m_indentLevel++;
+            result += formatLine(ifTrue);
+            m_indentLevel--;
+        }
 
-        if (!if_false.isEmpty())
+        if (!ifFalse.isEmpty())
         {
-            result += "else " + if_false;
+            if (ifTrueBlock)
+                result += "else";
+            else
+                result += formatLine("else", false);
+
+            if (ifFalseBlock) {
+                result += " " + ifFalse;
+            } else {
+                result += "\n";
+                m_indentLevel++;
+                result += formatLine(ifFalse, false);
+                m_indentLevel--;
+            }
         }
 
         return result;
@@ -537,8 +575,13 @@ QString DumpAstVisitor::parseStatement(Statement *statement, bool blockHasNext,
     case Node::Kind_WhileStatement: {
         auto *whileStatement = cast<WhileStatement *>(statement);
 
-        return "while ("+parseExpression(whileStatement->expression) + ") "
-                + parseStatement(whileStatement->statement);
+        m_blockNeededBraces = false;
+
+        auto statement = parseStatement(whileStatement->statement, false, true);
+
+        return "while ("+parseExpression(whileStatement->expression) + ")"
+                + (m_blockNeededBraces ? " " : "")
+                + statement;
     }
     case Node::Kind_DoWhileStatement: {
         auto *doWhileStatement = cast<DoWhileStatement *>(statement);
@@ -758,6 +801,7 @@ bool DumpAstVisitor::visit(UiObjectDefinition *node) {
 void DumpAstVisitor::endVisit(UiObjectDefinition *node) {
     m_indentLevel--;
     addLine(m_inArrayBinding && m_lastInArrayBinding != node ? "}," : "}");
+    addLine(getComment(node, Comment::Location::Back));
     if (!m_inArrayBinding)
         addNewLine();
 }
@@ -809,7 +853,8 @@ bool DumpAstVisitor::visit(UiScriptBinding *node) {
         else
             addNewLine();
 
-        m_firstBinding = false;
+        if (parseUiQualifiedId(node->qualifiedId) != "id")
+            m_firstBinding = false;
     }
 
     addLine(getComment(node, Comment::Location::Front));
@@ -834,6 +879,12 @@ bool DumpAstVisitor::visit(UiArrayBinding *node) {
     m_indentLevel++;
     m_inArrayBinding = true;
 
+    m_firstOfAll = true;
+    m_firstObject = true;
+    m_firstSignal = true;
+    m_firstBinding = true;
+    m_firstProperty = true;
+
     for (auto *item = node->members; item != nullptr; item = item->next) {
         if (item->next == nullptr)
             m_lastInArrayBinding = item->member;
@@ -883,6 +934,8 @@ bool DumpAstVisitor::visit(UiObjectBinding *node) {
 
     if (node->hasOnToken)
         result += " on "+parseUiQualifiedId(node->qualifiedId);
+    else
+        result.prepend(parseUiQualifiedId(node->qualifiedId) + ": ");
 
     addNewLine();
     addLine(getComment(node, Comment::Location::Front));
@@ -893,9 +946,10 @@ bool DumpAstVisitor::visit(UiObjectBinding *node) {
     return true;
 }
 
-void DumpAstVisitor::endVisit(UiObjectBinding *) {
+void DumpAstVisitor::endVisit(UiObjectBinding *node) {
     m_indentLevel--;
     addLine("}");
+    addLine(getComment(node, Comment::Location::Back));
 
     addNewLine();
 }
@@ -906,7 +960,7 @@ bool DumpAstVisitor::visit(UiImport *node) {
     QString result = "import ";
 
     if (!node->fileName.isEmpty())
-        result += node->fileName;
+        result += escapeString(node->fileName.toString());
     else
         result += parseUiQualifiedId(node->importUri);
 
diff --git a/tools/qmlformat/dumpastvisitor.h b/tools/qmlformat/dumpastvisitor.h
index 1348ee62e7..67e45d4bb3 100644
--- a/tools/qmlformat/dumpastvisitor.h
+++ b/tools/qmlformat/dumpastvisitor.h
@@ -81,7 +81,7 @@ private:
     QString getOrphanedComments(Node *node) const;
 
     QString parseStatement(Statement *statement, bool blockHasNext=false,
-                           bool blockAllowBraceless=true);
+                           bool blockAllowBraceless = false);
     QString parseStatementList(StatementList *list);
 
     QString parseExpression(ExpressionNode *expression);