Xmlconsole #52

barun511 · 2017-03-24T12:56:11Z

In Progress: Teaser Task for Gsoc - Sending raw xml via a "Send XML" button.

Steps:

Done

Create SendXML button.
Create a QTextEdit instance that pops up on button click.
Create a Class (QtXMLSenderWidget) that holds a textedit and a button to send the raw XML.

Not Done
4. On Button press, send XML in QtXMLSenderWidget.
5. Check for validity of XML before sending (optional)

Screenshots of work so far attached (only User Interface).

Test-Information: Built on LinuxMint 18 with Qt 5.5.1 successfully. License: This patch is BSD-licensed, see Documentation/Licenses/BSD-simplified.txt for details.

Test-information: Tested on LinuxMint 18, Qt 5.5.1

tfar · 2017-03-24T13:21:36Z

I think an input field in the original dialog would be preferred over opening an addition window to input the XML. Other than that it look great.

tfar

Left some code style comments. Thanks for working on this.

tfar · 2017-03-24T13:22:23Z

Swift/QtUI/QtXMLConsoleWidget.cpp

 setWindowTitle(tr("Debug Console"));
 emit titleUpdated();
 }

 QtXMLConsoleWidget::~QtXMLConsoleWidget() {
 }

+void QtXMLConsoleWidget::showWindow() {
+ QtXMLSenderWidget* XMLWindow = new QtXMLSenderWidget();


You can use auto here on the left-hand side.

tfar · 2017-03-24T13:26:22Z

Swift/QtUI/QtXMLSenderWidget.cpp

+ * Copyright (c) 2010-2016 Isode Limited.
+ * All rights reserved.
+ * See the COPYING file for more information.
+ */


If this file is completely new and written by yourself, it should have a copyright header with your name on it. See for example how another student has done it in their PR https://github.com/swift/swift/pull/46/files .

tfar · 2017-03-24T13:30:06Z

Swift/QtUI/QtXMLSenderWidget.cpp

+#include <Swift/QtUI/QtXMLSenderWidget.h>
+
+#include <string>
+#include <QString>


We tend to group our includes, so ideally the QString should be grouped with other Qt includes. There is also a script doing this for you, see the python script https://github.com/swift/swift/blob/master/BuildTools/FixIncludes.py .

barun511 · 2017-03-25T09:30:47Z

Changed to input field in original dialog, and changed copyright.

I'm having trouble figuring out how to send the outgoing XML, any tips on what code I should be looking at?

tfar · 2017-03-27T05:49:04Z

The MainController should have a Swift::Client instance. It has sendMessage() and sendPresence) method. For sending IQs you can also get the IQRouter from that Swift::Client instance. They are all defined in Swift::CoreClient https://github.com/swift/swift/blob/master/Swiften/Client/CoreClient.h .

tfar · 2017-03-27T08:21:58Z

Swift/QtUI/QtXMLConsoleWidget.cpp

-void QtXMLConsoleWidget::showWindow() {
- QtXMLSenderWidget* XMLWindow = new QtXMLSenderWidget();
- XMLWindow->show();
+void QtXMLConsoleWidget::showWindow(bool showXMLSender) {


While this way is totally fine, you can also use C++11 lambdas as Qt slots. This has the widget definition and the handler for the widget close to each other in the code and doesn't require an additional method to be added to the class. See https://github.com/swift/swift/blob/master/Swift/QtUI/QtBlockListEditorWindow.cpp#L113 for an example.

Also made this change :)

tfar · 2017-03-27T08:27:54Z

Swift/QtUI/QtXMLConsoleWidget.cpp

+ else
+ {
+ XMLWindow->hide();
+ }


Instead of this if you could also pass a boolean flag to QWidget::setVisible method, see http://doc.qt.io/qt-5/qwidget.html#visible-prop .

Made change. Will push when I get back to college tomorrow :)

tfar · 2017-03-27T08:29:08Z

Swift/QtUI/QtXMLConsoleWidget.h

@@ -42,5 +42,7 @@ namespace Swift {
 private:
 QTextEdit* textEdit;
 QCheckBox* enabled;
+ QCheckBox* debugenabled;


Ideally all pointers are initialised with nullptr if you don't directly assign a correct value to them at declaration time.

I'm not entirely sure what you mean, could I have a bit more detail?

The value of these members is undefined until you assign a value to them.
You can write QCheckBox* debugEnabled = nullptr in the class definition in the header file to do so. Furthermore it should be debugEnabled and not debugenabled, as we camelCase for member variables. See the Naming section in https://github.com/swift/swift/wiki/Coding-Style .

barun511 · 2017-04-01T18:04:50Z

On button press, Send XML <-- Done

tfar

Left some inline feedback. The general direction is good. Please run the FixIncludes.py tool over the touched files or sort/group the includes manually.

tfar · 2017-04-03T09:38:58Z

Swift/Controllers/UIInterfaces/XMLConsoleWidget.h

@@ -7,6 +7,7 @@
 #pragma once

 #include <Swiften/Base/SafeByteArray.h>
+#include <boost/signals2.hpp>


In Swift we aim to keep our includes sorted. There's a handy script for this which aims to do the job for most cases, https://github.com/swift/swift/blob/master/BuildTools/FixIncludes.py . Simply run it like ./BuildTools/FixIncludes.py -i path_to_cpp_or_h_file.cpp.

tfar · 2017-04-03T09:41:34Z

Swift/Controllers/XMLConsoleController.cpp

 }
+}


There is a newline missing at the end of the file. While it's not required for C++11, currently all files in Swift have newlines at the end of the source files and our CI enforces this.

tfar · 2017-04-03T09:42:54Z

Swift/Controllers/XMLConsoleController.cpp

+ this->client_ = client_;
+}
+void XMLConsoleController::sendXML(std::string data){
+ client_->sendData(data);


This should probably check if client_ is actually set, not? If the developer forgets to call setClient(…) pressing the send XML button would cause a crash I think.

tfar · 2017-04-03T09:44:26Z

Swift/Controllers/XMLConsoleController.h

@@ -12,13 +12,15 @@
 #include <boost/signals2.hpp>

 #include <Swiften/Base/SafeByteArray.h>
+#include <Swiften/Client/Client.h>


If the predeclaration below is sufficient, then there's no need to include Client.h here, right? If not, the predeclaration is not needed anymore.

tfar · 2017-04-03T09:48:51Z

Swift/QtUI/QtXMLConsoleWidget.h

@@ -42,5 +42,7 @@ namespace Swift {
 private:
 QTextEdit* textEdit;
 QCheckBox* enabled;
+ QCheckBox* debugenabled;


The value of these members is undefined until you assign a value to them.
You can write QCheckBox* debugEnabled = nullptr in the class definition in the header file to do so. Furthermore it should be debugEnabled and not debugenabled, as we camelCase for member variables. See the Naming section in https://github.com/swift/swift/wiki/Coding-Style .

tfar · 2017-04-03T09:50:15Z

Swift/QtUI/QtXMLSenderWidget.cpp

+ * Copyright (c) 2017 Barun Parruck.
+ * All rights reserved.
+ * See the COPYING file for more information.
+ */


Your copyright header should match the license you aim to submit the patch under. See for example cc873b3#diff-0856e1d938fb0b1106ce0267bbb319b0R1 .

tfar · 2017-04-03T09:59:53Z

Swift/QtUI/QtXMLSenderWidget.h

@@ -0,0 +1,28 @@
+/*


Is an additional widget really needed in this case, or wouldn't it be more straightforward to just put QTextEdit in the existing QtXMLConsoleWidget class. I think that would make the code a bit cleaner.

barun511 · 2017-04-04T09:29:46Z

I've made the necessary changes :) Anything else?

tfar

Okay. I've build the app and ran it locally. Ideally I'd put
the checkbox and the send XML button at the bottom, next to the "Trace input/output" checkbox. Leaving space between all these and the clear button.

Furthermore I'd live the log at the top and the input below, ideally with a label above, maybe put it in a QGroupBox.

If i send some basic XML like "" I don't see it appear in the XML log. Is this code already hooked up?

Some smaller comments inline.

tfar · 2017-04-04T11:58:40Z

Swift/Controllers/XMLConsoleController.cpp

@@ -41,5 +43,13 @@ void XMLConsoleController::handleDataWritten(const SafeByteArray& data) {
 xmlConsoleWidget->handleDataWritten(data);
 }
 }
-
+void XMLConsoleController::setClient(std::shared_ptr<Client> client_)


Parameter should not have the _ suffix. This way you won't need to use this in the next line too.

tfar · 2017-04-04T11:59:34Z

Swift/Controllers/XMLConsoleController.cpp

@@ -41,5 +43,13 @@ void XMLConsoleController::handleDataWritten(const SafeByteArray& data) {
 xmlConsoleWidget->handleDataWritten(data);
 }
 }
-
+void XMLConsoleController::setClient(std::shared_ptr<Client> client_)
+{


This { belongs at the end of the previous line.

tfar · 2017-04-04T12:00:07Z

Swift/Controllers/XMLConsoleController.cpp

+{
+ this->client_ = client_;
+}
+void XMLConsoleController::sendXML(std::string data){


Please add a newline between method definitions.

tfar · 2017-04-04T12:01:00Z

Swift/Controllers/XMLConsoleController.h


 private:
 void handleUIEvent(std::shared_ptr<UIEvent> event);
-
+ void sendXML(std::string data);


The parameter should have the type const std::string& to avoid a copy.

Kev · 2017-04-04T15:11:16Z

Hi there, I've not looked at this yet, is it possible to get a screenshot of how the current input dialog looks, and what the button in the XML window looks like, please?

barun511 · 2017-04-04T15:18:56Z

Kev · 2017-04-04T15:34:58Z

Great, thanks. Some comments on that:
Sending XML is a very rare thing to want to do, so we shouldn't clutter the debug dialog with it directly, but hide it behind a button (or is that what manually enter XML does? If so, it should probably have a better name, like 'show XML entry tools' or somesuch.

It looks like at the moment it's only letting you put in a block of XML on its own, and doesn't have any mechanisms for choosing what stanza type you want to be sending, what the values of to/from/type/id should be?

barun511 · 2017-04-04T15:36:11Z

(That is what manually enter XML does, I'll change the name).

No mechanisms for now, just the raw stuff.

Kev · 2017-04-04T15:37:19Z

Thanks. It's probably not worth us spending much time reviewing the patch until the functionality is there, is it?

barun511 · 2017-04-04T15:46:54Z

Well, you can still send raw XML (or even non-XML), just not specific types yet. I'm more than happy to also hold off on reviewing until I add more functionality (though I'm really grateful to Tobias for reviewing, it's helped me fix a number of issues I wouldn't have been aware of).

tfar

Code stylistic the latest version is much better. Ideally we want to send Presence/IQ/Message stanzas (the C++ classes with the same names) with a RawXMLPayload ( https://github.com/swift/swift/blob/master/Swiften/Elements/RawXMLPayload.h ) as a child. That way we can call the correct method on the Client object (sendMessage(), sendPresence() and getIQRouter()->sendIQ(). This way it won't break XEP-0198 support.

The input form for the stanza could look like https://www.dropbox.com/s/wrbsb1ushi9zotp/Screenshot%202017-04-12%2012.15.06.png?dl=0 . This way the user can select a stanza type, specify the IQ/Presence/Message type, to, from and id. Plus the free form RawXMLPayload inside of it.

Let me know if you have further questions.

barun511 added 2 commits March 21, 2017 15:38

Fix swift#43 : Add LinuxMint to supported distros

b228a54

Test-Information: Built on LinuxMint 18 with Qt 5.5.1 successfully. License: This patch is BSD-licensed, see Documentation/Licenses/BSD-simplified.txt for details.

Add Debug XML button and window to send xml

8c0fda7

Test-information: Tested on LinuxMint 18, Qt 5.5.1

tfar reviewed Mar 24, 2017

View reviewed changes

Add XMLSenderWidget to XMLConsole

4d1a2ba

tfar reviewed Mar 27, 2017

View reviewed changes

barun511 added 2 commits March 27, 2017 16:18

Make recommended changes post code review

41858f0

Make the send XML button functional

c27fc83

tfar reviewed Apr 3, 2017

View reviewed changes

barun511 added 2 commits April 4, 2017 13:19

Remove unnecessary QtXMLSenderWidget

dc84110

Fixes a grouped include

d682720

tfar reviewed Apr 4, 2017

View reviewed changes

barun511 added 3 commits April 7, 2017 14:03

Change "Manually enter" to "Show XML entry tools"

85dd4b3

Make UI changes suggested by Tobias

3c6383a

Make stylistic changes suggested by Tobias

3aaaa61

tfar reviewed Apr 12, 2017

View reviewed changes

Xmlconsole #52

Are you sure you want to change the base?

Xmlconsole #52

Conversation

barun511 commented Mar 24, 2017

tfar commented Mar 24, 2017

tfar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barun511 commented Mar 25, 2017

tfar commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barun511 commented Apr 1, 2017

tfar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barun511 commented Apr 4, 2017

tfar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kev commented Apr 4, 2017

barun511 commented Apr 4, 2017

Kev commented Apr 4, 2017

barun511 commented Apr 4, 2017

Kev commented Apr 4, 2017

barun511 commented Apr 4, 2017

tfar left a comment

Choose a reason for hiding this comment