[core, node, darwin, qt] Remove support for paint classes #8953

jfirebaugh · 2017-05-10T22:15:29Z

We've long talked about doing this, and with the implementation of async rendering, which will unlock smart set style, I think it's time.

Fixes #2875.

1ec5

Keeping the methods but making them nonfunctional would technically allow us to avoid a major semver bump on iOS, but it’s against the spirit of those rules to change the behavior in a significant way. Fortunately, hopefully style class usage on iOS is insignificant enough to make this change regardless.

We should state in the documentation comments for the affected methods and in the iOS/macOS changelogs that these methods have no effect. (There are also still deprecated style class methods on the iOS implementation of MGLMapView.) Finally, there’s a line in the “Information for Style Authors” guide that refers to style classes; we should probably remove that.

1ec5 · 2017-05-12T18:43:19Z

platform/darwin/src/MGLStyle.mm

 }

 - (BOOL)hasStyleClass:(NSString *)styleClass
 {
-    return styleClass && self.mapView.mbglMap->hasClass([styleClass UTF8String]);
+    return false;


The negative version of BOOL is NO, not false.

1ec5 · 2017-05-12T18:43:38Z

platform/darwin/src/MGLStyle.mm

-    }
-
-    return returnArray;
+    return [NSMutableArray arrayWithCapacity:0];


Use @[] for an empty array.

1ec5

☀️

1ec5 · 2017-05-12T21:41:50Z

platform/darwin/src/MGLStyle.h

 */
- (void)addStyleClass:(NSString *)styleClass __attribute__((deprecated("This method will be removed in a future release.")));
+- (void)addStyleClass:(NSString *)styleClass __attribute__((deprecated("This method is non-functional.")));


Normally deprecation messages say what to use instead, or they say, “There is no replacement.” In this case, the replacement would be setting properties on style layers individually, right?

brunoabinader · 2017-05-15T10:35:39Z

platform/qt/src/qmapboxgl.cpp

 */
-void QMapboxGL::addClass(const QString &className)
+void QMapboxGL::addClass(const QString &)


We don't need to maintain binary compatibility on the QMapboxGL interface - we use it internally and expose a separate API on the QtLocation plugin - thus it is safe to assume we can remove all class-handling function implementations from QMapboxGL.

brunoabinader · 2017-05-15T10:35:52Z

platform/qt/src/qmapboxgl.cpp

 */
-void QMapboxGL::removeClass(const QString &className)
+void QMapboxGL::removeClass(const QString &)


brunoabinader · 2017-05-15T10:35:59Z

platform/qt/src/qmapboxgl.cpp

 */
-bool QMapboxGL::hasClass(const QString &className) const
+bool QMapboxGL::hasClass(const QString &) const


brunoabinader · 2017-05-15T10:36:07Z

platform/qt/src/qmapboxgl.cpp

 */
-void QMapboxGL::setClasses(const QStringList &classNames)
+void QMapboxGL::setClasses(const QStringList &)


brunoabinader · 2017-05-15T10:36:17Z

platform/qt/src/qmapboxgl.cpp


    \deprecated
-    \sa setClasses()
 */
 QStringList QMapboxGL::getClasses() const


brunoabinader · 2017-05-15T10:37:47Z

platform/qt/src/qmapboxgl.cpp

@@ -1006,7 +984,7 @@ void QMapboxGL::setLayoutProperty(const QString& layer, const QString& property,
        map->setPaintProperty("route","line-dasharray", lineDashArray);
    \endcode
 */
-void QMapboxGL::setPaintProperty(const QString& layer, const QString& property, const QVariant& value, const QString& styleClass)
+void QMapboxGL::setPaintProperty(const QString& layer, const QString& property, const QVariant& value, const QString&)


For the same reasons previously mentioned, we can remove the final argument entirely.

brunoabinader

Awesome job @jfirebaugh 🚀 I appreciate your attention over binary compatibility on the Qt API though we don't need it - we can nuke that code away too 🤘

jfirebaugh · 2017-05-15T15:19:45Z

Great, thanks @brunoabinader, I will remove it.

It's still relevant for runtime-styling driven paint property changes.

jfirebaugh force-pushed the rm-classes branch 4 times, most recently from 97b67ff to 420e83a Compare May 10, 2017 23:53

jfirebaugh changed the title ~~[core, node, darwin] Remove support for paint classes~~ [core, node, darwin, qt] Remove support for paint classes May 10, 2017

jfirebaugh force-pushed the rm-classes branch 2 times, most recently from 0d1325d to b1075f5 Compare May 11, 2017 01:22

jfirebaugh force-pushed the immutable branch from 8f8710e to 7b2e722 Compare May 11, 2017 15:54

jfirebaugh force-pushed the rm-classes branch from b1075f5 to 574bdcc Compare May 11, 2017 19:49

jfirebaugh force-pushed the immutable branch from 3db6ac8 to 38d1677 Compare May 11, 2017 20:07

jfirebaugh force-pushed the rm-classes branch from 574bdcc to 81dd4ea Compare May 11, 2017 20:41

jfirebaugh force-pushed the immutable branch 4 times, most recently from e859904 to 2228ad9 Compare May 12, 2017 17:55

jfirebaugh force-pushed the rm-classes branch from 81dd4ea to c02315b Compare May 12, 2017 18:28

jfirebaugh changed the base branch from immutable to master May 12, 2017 18:28

jfirebaugh requested review from ivovandongen, 1ec5 and brunoabinader May 12, 2017 18:28

1ec5 suggested changes May 12, 2017

View reviewed changes

1ec5 approved these changes May 12, 2017

View reviewed changes

brunoabinader reviewed May 15, 2017

View reviewed changes

platform/qt/src/qmapboxgl.cpp Outdated

\deprecated

\sa setClasses()

*/

QStringList QMapboxGL::getClasses() const

Copy link

Member

brunoabinader May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

brunoabinader reviewed May 15, 2017

View reviewed changes

brunoabinader suggested changes May 15, 2017

View reviewed changes

jfirebaugh added 4 commits May 15, 2017 08:21

[core] Merge light_properties.hpp into light_impl.hpp

fa73a2a

[core] *LayerImpl::cascading ⇢ paint

f2c4743

[core] Unify {Paint,Layout,Light}Properties

73c7e45

[core] cascade ⇢ transition / cascading ⇢ transitionable

7beedb0

jfirebaugh force-pushed the rm-classes branch from d70cfcf to 755c4d7 Compare May 15, 2017 15:27

brunoabinader approved these changes May 15, 2017

View reviewed changes

jfirebaugh added 2 commits May 15, 2017 08:50

[core, node, darwin, qt] Remove support for paint classes

d0ed7f6

[qt] Un-deprecate QMapboxGL::setTransitionOptions

918f2e1

It's still relevant for runtime-styling driven paint property changes.

jfirebaugh force-pushed the rm-classes branch from 755c4d7 to 918f2e1 Compare May 15, 2017 15:50

jfirebaugh merged commit 244fdaa into master May 15, 2017

jfirebaugh deleted the rm-classes branch May 15, 2017 16:45

This was referenced May 15, 2017

Remove integration tests using paint classes mapbox/mapbox-gl-js#4714

Merged

[test] Update gl-js submodule #9004

Merged

1ec5 mentioned this pull request Jul 5, 2017

Merge release-ios-v3.6.0-android-v5.1.0 #9416

Merged

friedbunny mentioned this pull request Sep 28, 2017

[ios] Expose -showAttribution publicly as an IBAction #10085

Merged

This was referenced Mar 21, 2018

Remove remaining style classes references #11495

Open

Prevent/hide map features from rendering #11477

Closed

friedbunny mentioned this pull request Jun 25, 2019

[ios] Remove previously deprecated unavailable methods and properties #15000

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core, node, darwin, qt] Remove support for paint classes #8953

[core, node, darwin, qt] Remove support for paint classes #8953

jfirebaugh commented May 10, 2017 •

edited

Loading

1ec5 left a comment •

edited

Loading

1ec5 May 12, 2017

1ec5 May 12, 2017

1ec5 left a comment

1ec5 May 12, 2017

brunoabinader May 15, 2017

brunoabinader May 15, 2017

brunoabinader May 15, 2017

brunoabinader May 15, 2017

brunoabinader May 15, 2017

brunoabinader May 15, 2017

brunoabinader left a comment

jfirebaugh commented May 15, 2017

[core, node, darwin, qt] Remove support for paint classes #8953

[core, node, darwin, qt] Remove support for paint classes #8953

Conversation

jfirebaugh commented May 10, 2017 • edited Loading

1ec5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 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

brunoabinader left a comment

Choose a reason for hiding this comment

jfirebaugh commented May 15, 2017

jfirebaugh commented May 10, 2017 •

edited

Loading

1ec5 left a comment •

edited

Loading