Clean WMS service #4019

dmarteau · 2017-01-18T12:35:31Z

- Remove "transitional" code
- Remove duplicated code (or functionaly equivalent code)
- Remove dead code
- Sanitize error checking
- Ensure correct build with HAVE_SERVER_PYTHON_PLUGINS not defined

rldhont · 2017-01-18T16:59:05Z

@elpaso any objection to merge ?

elpaso · 2017-01-18T17:27:40Z

@rldhont not at all, I've just added some minor comments

elpaso

Looks good to me. I've just added some minor notes wanted to make sure that @sbrunner is fine about removing the conditional for the HAVE_SERVER_PYTHON_PLUGINS

elpaso · 2017-01-18T14:58:49Z

src/server/services/wms/qgsmaprendererjobproxy.cpp

@@ -27,15 +27,11 @@ namespace QgsWms
  QgsMapRendererJobProxy::QgsMapRendererJobProxy(
    bool parallelRendering
    , int maxThreads
-#ifdef HAVE_SERVER_PYTHON_PLUGINS


Not sure: @sbrunner what do you think?
BTW, always make sure that id builds with plugins disabled.

@elpaso If HAVE_SERVER_PYTHON_PLUGINS is not defined then accessControl is an opaque type and the pointer returned by the serverInterface in set to null: by doing this we can safely remove all conditionals on parameter list (but not those associated to specific code execution).

My wish is to remove all HAVE_SERVER_PYTHON_PLUGINS defines from parameters list: they are ugly and they require to have them at every invocation of the method or function.

@elpaso checked that build is ok with plugins disabled. Removed HAVE_SERVER_PYTHON_PLUGINS defines from parameters list and fixed other defines accordingly.

thanks @dmarteau, the refactoring is looking great!

elpaso · 2017-01-18T15:03:11Z

src/server/services/wms/qgswmsdescribelayer.cpp


+    if ( !parameters.contains( QStringLiteral( "SLD_VERSION" ) ) )


I'm not an expert here, but I suspect that if the function has an overload for QLatin1String it would be faster here. (see: https://woboq.com/blog/qstringliteral.html)
Same in other points of the code.

I read the post: the assertion is very vague and gives no hints about wich kind of algorithm is impacted by the choice of one are another type: In our case it would be the computation of the order relation between two strings ( it's a QMap ). I would bet that the fastest case would be when the compared strings are of the same type.
Afaik: nothing can prevent parameters to be utf-8 so if had to choose a type I would use the most common case everywhere.

@elpaso Afaik: QMap has no specific overload for the contains method.

elpaso · 2017-01-18T15:22:03Z

src/server/services/wms/qgswmsgetcapabilities.cpp

+    QDomProcessingInstruction xmlDeclaration = doc.createProcessingInstruction( QStringLiteral( "xml" ),
+        QStringLiteral( "version=\"1.0\" encoding=\"utf-8\"" ) );
+
+    // Appond format helper


elpaso · 2017-01-18T15:24:30Z

src/server/services/wms/qgswmsgetcapabilities.cpp

+    elem.appendChild( dcpTypeElement.cloneNode().toElement() ); //this is the same as for 'GetCapabilities'
+    requestElement.appendChild( elem );
+
+    if ( projectSettings ) //remove composer templates from GetCapabilities in the long term


What do you mean with the comment? Do you want to add a different REQUEST for getProjectSettings ?

I don't know: these are original comments. Functionaly nothing has changed, I have only restructured parts of the original code.

- Remove "transitional" code - Remove duplicated code (or functionaly equivalent code) - Remove dead code - Sanitize error checking - Ensure correct build with HAVE_SERVER_PYTHON_PLUGINS not defined

dmarteau · 2017-01-19T09:06:51Z

@elpaso speaking about optimisation: is there something planned about Q_FOREACH expression ?
see https://www.kdab.com/goodbye-q_foreach/

elpaso · 2017-01-19T09:16:50Z

I'd ask @nyalldawson and @m-kuhn how to proceed, if I get it right we should eventually replace Q_FOREACH.

m-kuhn · 2017-01-19T11:38:36Z

Short: I am not sure.

Long: In this kdab post they advise to get rid of Q_FOREACH in favor of range-based for-loops (for item : container). There was a lot of controversy about it. Basically because the range-based-for-loop does require additional code to make use of implicit sharing in Qt containers. This makes porting harder and future usage less straightforward.

There is also an issue with our builtin astyle which should be updated before we should make use of range-based for-loops.

I appreciate if we stay close to the Qt style of coding and don't use deprecated concepts, but this is really a tricky one.

luipir · 2017-01-19T12:32:35Z

generally speaking I'm in favor to reduce the use of QT dependent operators.

see feature request qgis#4019

dmarteau force-pushed the wms_cleanup branch from 9fa09fd to 9b7cf15 Compare January 18, 2017 12:47

sbrunner approved these changes Jan 18, 2017

View reviewed changes

dmarteau force-pushed the wms_cleanup branch from 9b7cf15 to 8aeef71 Compare January 18, 2017 13:05

rldhont requested review from elpaso and mhugent January 18, 2017 13:09

rldhont added the Server label Jan 18, 2017

rldhont added this to the QGIS 3 milestone Jan 18, 2017

rldhont approved these changes Jan 18, 2017

View reviewed changes

elpaso requested changes Jan 18, 2017

View reviewed changes

dmarteau force-pushed the wms_cleanup branch 2 times, most recently from b68e1af to 71564a2 Compare January 18, 2017 22:35

Clean WMS service

a80497e

- Remove "transitional" code - Remove duplicated code (or functionaly equivalent code) - Remove dead code - Sanitize error checking - Ensure correct build with HAVE_SERVER_PYTHON_PLUGINS not defined

dmarteau force-pushed the wms_cleanup branch from 71564a2 to a80497e Compare January 18, 2017 23:09

elpaso approved these changes Jan 19, 2017

View reviewed changes

rldhont merged commit 53752ba into qgis:master Jan 19, 2017

dmarteau deleted the wms_cleanup branch January 19, 2017 09:46

dmarteau mentioned this pull request Jan 27, 2017

[server] WIP clean project parsing by using QgsProject #3942

Merged

stevenmizuno added a commit to stevenmizuno/QGIS that referenced this pull request Jan 11, 2018

[feature] allow Python Console to be a dockwidget or top-level window

83062e6

see feature request qgis#4019

qgib mentioned this pull request May 24, 2019

Identify results - previous state (location, docked status, size) is not saved #14113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean WMS service #4019

Clean WMS service #4019

dmarteau commented Jan 18, 2017 •

edited

Loading

rldhont commented Jan 18, 2017

elpaso commented Jan 18, 2017

elpaso left a comment

elpaso Jan 18, 2017

dmarteau Jan 18, 2017 •

edited

Loading

dmarteau Jan 18, 2017 •

edited

Loading

elpaso Jan 19, 2017

elpaso Jan 18, 2017

dmarteau Jan 18, 2017 •

edited

Loading

dmarteau Jan 18, 2017 •

edited

Loading

elpaso Jan 18, 2017

elpaso Jan 18, 2017

dmarteau Jan 18, 2017

dmarteau commented Jan 19, 2017

elpaso commented Jan 19, 2017

m-kuhn commented Jan 19, 2017

luipir commented Jan 19, 2017


		if ( !parameters.contains( QStringLiteral( "SLD_VERSION" ) ) )

Clean WMS service #4019

Clean WMS service #4019

Conversation

dmarteau commented Jan 18, 2017 • edited Loading

rldhont commented Jan 18, 2017

elpaso commented Jan 18, 2017

elpaso left a comment

Choose a reason for hiding this comment

elpaso Jan 18, 2017

Choose a reason for hiding this comment

dmarteau Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

dmarteau Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

elpaso Jan 19, 2017

Choose a reason for hiding this comment

elpaso Jan 18, 2017

Choose a reason for hiding this comment

dmarteau Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

dmarteau Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

elpaso Jan 18, 2017

Choose a reason for hiding this comment

elpaso Jan 18, 2017

Choose a reason for hiding this comment

dmarteau Jan 18, 2017

Choose a reason for hiding this comment

dmarteau commented Jan 19, 2017

elpaso commented Jan 19, 2017

m-kuhn commented Jan 19, 2017

luipir commented Jan 19, 2017

dmarteau commented Jan 18, 2017 •

edited

Loading

dmarteau Jan 18, 2017 •

edited

Loading

dmarteau Jan 18, 2017 •

edited

Loading

dmarteau Jan 18, 2017 •

edited

Loading

dmarteau Jan 18, 2017 •

edited

Loading