[Qt] Add more docs #7594

tmpsantos · 2017-01-04T16:53:34Z

Add even more docs to QMapboxGL.

mention-bot · 2017-01-04T16:53:36Z

@tmpsantos, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh to be potential reviewers.

1ec5 · 2017-01-04T21:32:18Z

platform/qt/src/qmapboxgl.cpp

 {
 d_ptr->mapObj->setLatLng(mbgl::LatLng { coordinate_.first, coordinate_.second }, d_ptr->margins);
 }

 /*!
- Convenience method for setting the \a coordinate and \a zoom simultaneously.
+ Convenience method for setting the \a coordinate_ and \a zoom_ simultaneously.


Public _ variables are kind of unfortunate. Any chance you could rename these variables and explicitly use this inside the implementation where there’s ambiguity?

That is true. I've been trying when possible and being creative with the names.

In this case, the Qt standard is to use coordinate() as a getter instead of getCoordinate() and this often overlaps with parameters names. For Qt itself it is not a problem because it doesn't care about parameter shadowing.

One solution would be do a post processing of the generated documentation or disable this warning for the Qt build.

Ah, that’s unfortunate. In Objective-C and Swift, it’s also possible for a getter and a variable to share the same namespace, and _ always denotes an internal implementation detail that shouldn’t be touched. So one old convention is to say e.g. theCoordinate or aZoom, but I don’t know if that looks reasonable in Qt code.

Is it possible to document this method on its declaration rather than its implementation? Then you can give the parameter two different names, coordinate in the header (which shouldn’t conflict with coordinate()) and coordinate_ here in the implementation.

Excellent idea, it works! We can get rid of the _.

1ec5 · 2017-01-04T21:33:58Z

platform/qt/src/qmapboxgl.cpp

+/*!
+ Sets the \a duration and \a delay of style class transitions. An animation
+ morphing a style property value to another value is performed when a new
+ class is set.


Use active voice:

Style property values transition to new values with animation when a new class is set.

1ec5 · 2017-01-04T21:39:51Z

platform/qt/src/qmapboxgl.cpp

+ This example hides the layer \b "route":
+
+ \code
+ map->setLayoutProperty("makerBackground", "visibility", "none");


Typo: maker → marker.

It should be "route" actually.

1ec5 · 2017-01-04T22:23:29Z

platform/qt/src/qmapboxgl.cpp

+ \value MapChangeRegionDidChangeAnimated Not in use by QMapboxGL.
+
+ \value MapChangeWillStartLoadingMap The map is getting loaded. This state
+ is set only once right after QMapboxGL is created and a style is set.


Link QMapboxGL?

qdoc links automatically in this case.

https://tmpsantos.com.br/qmapboxgl/qmapboxgl.html#MapChange-enum

Doh, I keep forgetting that tools other than jazzy can autolink without any markup. 👍

1ec5 · 2017-01-04T22:24:33Z

platform/qt/src/qmapboxgl.cpp

@@ -657,19 +716,30 @@ void QMapboxGL::setPitch(double pitch_)
 d_ptr->mapObj->setPitch(pitch_);
 }

+/*!
+ Returns the North Orientation mode.


Lowercase “north orientation”.

1ec5 · 2017-01-04T22:55:13Z

platform/qt/src/qmapboxgl.cpp

+ valid GL context is expected with an appropriated GL viewport state set
+ for the size of the canvas.
+
+ This function should be called only after the signal needsRendering() is


Is this documentation being generated by Doxygen or something along those lines? This sounds like a good case for Doxygen’s \pre command.

needsRendering() gets detected and linked by qtdoc automatically because it is a signal of the same class.

Sorry, I was referring to the paragraph as a whole, not the reference to needsRendering(). In Doxygen syntax, \pre means “precondition” and is given special prominence in the generated output.

1ec5 · 2017-01-04T22:56:02Z

platform/qt/src/qmapboxgl.cpp

@@ -1085,11 +1346,42 @@ void QMapboxGL::render()
 }
 #endif

+/*!
+ Informs the map that the network connection was established, causing
+ all timeouted network requests to retry immediately.


s/was established/has been established/
s/timeouted network requests/network requests that previously timed out/
s/to retry/to be retried/

1ec5 · 2017-01-04T22:56:36Z

platform/qt/src/qmapboxgl.cpp

+ \fn void QMapboxGL::copyrightsChanged(const QString &copyrightsHtml);
+
+ This signal is emitted when the copyrights of the current content of the map
+ has change. This can be caused by a style change or adding a new data source.


s/has change/have changed/

1ec5 · 2017-01-04T22:57:05Z

platform/qt/src/qmapboxgl.cpp

@@ -366,6 +366,54 @@ void QMapboxGLSettings::setAccessToken(const QString &token)
 */

 /*!
+ \enum QMapboxGL::MapChange
+
+ This enum represents the last changed occurred on the map state.


s/last change occurred on/last change that occurred to/

1ec5 · 2017-01-04T22:58:28Z

platform/qt/src/qmapboxgl.cpp

+ is set only once right after QMapboxGL is created and a style is set.
+
+ \value MapChangeDidFinishLoadingMap All the resources were loaded and parsed
+ and the map is fully rendered. This is the final state of the map.


“Final” implies that the state will never change again, doesn’t it? This enum value sort of represents a quiescent state, but the map state can continue changing if for instance the user starts panning.

Correct, I will reword this. What I was trying to say here is this is the most complete state of the map in terms of things rendered and resources loaded.

1ec5 · 2017-01-06T19:33:28Z

platform/qt/src/qmapboxgl.cpp

+ morphing a style property value to another value is performed when a new
+ class is set.
+
+ \deprecated


As far as I know, global transition options are not deprecated and not under consideration for removal.

1ec5 · 2017-01-06T19:37:00Z

platform/qt/src/qmapboxgl.cpp

+ as defined by \l {https://www.mapbox.com/mapbox-gl-style-spec/} {the vector tiles specification}
+ for paint properties.
+
+ The argument \a klass_ is deprecated and is used for defining the style class for the paint


My point was that you’d have two methods:

void setPaintProperty(const QString& layer, const QString& property, const QVariant& value); //! \deprecated void setPaintProperty(const QString& layer, const QString& property, const QVariant& value, const QString& klass) __attribute__((deprecated));

This works in C++03; is there a Qt-specific reason why we can’t overload the method like this?

tmpsantos · 2017-01-06T22:09:51Z

@1ec5 thanks a lot for pushing this documentation to the next level: https://tmpsantos.com.br/qmapboxgl/qmapboxgl.html

1ec5 · 2017-01-06T22:11:31Z

platform/qt/src/qmapboxgl.cpp

+ \endtable
+
+ For a giver layer \c route, setting the property \c line-dasharray that requires
+ a Mabpox Style type \b Array can be done as follows using a QVariantList:


s/giver/given/, and avoid passive voice:

If the style specification defines the property’s type as Array, use a QVariantList. For example, the following code sets a route layer’s line-dasharray property:

1ec5 · 2017-01-06T22:13:02Z

platform/qt/src/qmapboxgl.cpp

+ \endcode
+
+ This table describes the mapping between \l {https://www.mapbox.com/mapbox-gl-style-spec/#types}
+ {style types} and Qt types accepted by setLayoutProperty():


This table also applies to setPaintProperty(), so copy this table to that method’s documentation. (In particular, color-typed properties are paint properties.)

I did that ;)

But I'm going to remove Color.

They will never return true because Qt only does client side animations.

It propagated via copy & paste. :-P

Showing how to add numbers in an array.

brunoabinader

🎉

tmpsantos self-assigned this Jan 4, 2017

tmpsantos requested review from brunoabinader and lyzidiamond January 4, 2017 16:54

tmpsantos force-pushed the tmpsantos-qt_docs2 branch from c34306c to 2287b04 Compare January 4, 2017 17:06

tmpsantos requested a review from 1ec5 January 4, 2017 17:29

tmpsantos added the Qt Mapbox Maps SDK for Qt, aka Qt Location Mapbox GL label Jan 4, 2017

tmpsantos force-pushed the tmpsantos-qt_docs2 branch from 2287b04 to a8501dc Compare January 4, 2017 18:57

tmpsantos mentioned this pull request Jan 4, 2017

[Qt] Add docs for QMapboxGL #7571

Merged

1ec5 reviewed Jan 4, 2017

View reviewed changes

1ec5 reviewed Jan 6, 2017

View reviewed changes

tmpsantos force-pushed the tmpsantos-qt_docs2 branch 2 times, most recently from 82d1148 to 07ad792 Compare January 6, 2017 22:07

1ec5 reviewed Jan 6, 2017

View reviewed changes

[Qt] Removed unused methods

e301b0c

They will never return true because Qt only does client side animations.

tmpsantos force-pushed the tmpsantos-qt_docs2 branch from 07ad792 to 6b593fc Compare January 6, 2017 22:18

tmpsantos added 3 commits January 6, 2017 21:08

[Qt] Add more docs

a15b388

[Qt] Fixed typo s/maker/marker

bdd00c7

It propagated via copy & paste. :-P

[Qt] Added line-dasharray to the example app

d341a2f

Showing how to add numbers in an array.

tmpsantos force-pushed the tmpsantos-qt_docs2 branch from 6b593fc to d341a2f Compare January 6, 2017 23:09

brunoabinader approved these changes Jan 9, 2017

View reviewed changes

tmpsantos merged commit eaf03f4 into master Jan 9, 2017

tmpsantos deleted the tmpsantos-qt_docs2 branch January 9, 2017 15:49

brunoabinader mentioned this pull request Jan 26, 2017

Add user-facing Qt SDK docs #6834

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Qt] Add more docs #7594

[Qt] Add more docs #7594

tmpsantos commented Jan 4, 2017

mention-bot commented Jan 4, 2017

1ec5 Jan 4, 2017

tmpsantos Jan 6, 2017

1ec5 Jan 6, 2017

tmpsantos Jan 6, 2017

1ec5 Jan 4, 2017

1ec5 Jan 4, 2017

tmpsantos Jan 6, 2017

1ec5 Jan 4, 2017

tmpsantos Jan 6, 2017

1ec5 Jan 6, 2017 •

edited

Loading

1ec5 Jan 4, 2017

1ec5 Jan 4, 2017

tmpsantos Jan 6, 2017

1ec5 Jan 6, 2017 •

edited

Loading

1ec5 Jan 4, 2017

1ec5 Jan 4, 2017

1ec5 Jan 4, 2017

1ec5 Jan 4, 2017

tmpsantos Jan 6, 2017

1ec5 Jan 6, 2017

1ec5 Jan 6, 2017

tmpsantos commented Jan 6, 2017

1ec5 Jan 6, 2017 •

edited

Loading

1ec5 Jan 6, 2017

tmpsantos Jan 6, 2017

brunoabinader left a comment

[Qt] Add more docs #7594

[Qt] Add more docs #7594

Conversation

tmpsantos commented Jan 4, 2017

mention-bot commented Jan 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Jan 6, 2017 • edited Loading

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

1ec5 Jan 6, 2017 • edited Loading

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

tmpsantos commented Jan 6, 2017

1ec5 Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoabinader left a comment

Choose a reason for hiding this comment

1ec5 Jan 6, 2017 •

edited

Loading

1ec5 Jan 6, 2017 •

edited

Loading

1ec5 Jan 6, 2017 •

edited

Loading