Add missing getters/setters for most of the properties in AS::Component by grulja · Pull Request #117 · ximion/appstream

grulja · 2017-06-14T09:16:15Z

Note: only two of them should be missing (launchables and languages), because
we don't have wrappers for them yet

ximion · 2017-06-14T15:00:07Z

qt/bundle.h

+        /**
+         * \returns the internally stored AsBundle
+         */
+        _AsBundle *asBundle() const;


Are these really necessary? I would argue that this is only needed in case the Qt/C++ interface is incomplete and someone therefore wants to use the C library directly. In that case, we should rather fix the Qt binding to include the missing stuff, and not give library users the means to hack around it and not report the issue.

I use these internally, i.e. in as_component_add_icon() or as_component_add_screenshot() and so on, because those methods takes AsIcon or AsScreenshot only.

Then we should make the symbols internal and hide them from the public API... The tricky bit is that they're class members...
(I will take a look on whether it's possible to even have them as internal members, probably the only way to do that is to make them private and use friend classes for access. - in any case, just leave this as it is for now :-) )

ximion · 2017-06-14T15:01:45Z

Looks good on a first quick look at it, except for the obvious CI failure (which hints at an actual bug ^^).

Thank you for working on this, this is great help!

ximion · 2017-06-14T15:02:36Z

qt/component.h

+        bool insertCustomValue(const QString& key, const QString& value);
+
+        /**
+         * TODO contentRating, launchable and languages


Adding launchable to the Qt interface is actually a must-be-present feature for the next release, as that's a key new feature and using desktopId is deprecated in favor of it.

Ok, I will add them later on. I'm now working on AS::Metadata class needed for parsing.

OK, awesome! I was going to do this when I'm done with the parser refactoring, but it looks like that will still tle.

I've implemented above mentioned missing getters/setters including new classes needed for that.

Nice! Now, fixing the CI failure would be useful, otherwise a lot of code won't work ^^

Fixed, sorry, I haven't seen the error before. I saw CI failure at the beginning but there was a different problem (something with downloading or so) and I ignored it since then.

ximion · 2017-06-16T11:05:14Z

qt/component.cpp

+
+QStringList AppStream::Component::searchTokens() const
+{
+    valueWrap(as_component_get_search_tokens(m_cpt));


The CI complains about a missing return statement here...
And I was also surprised that this thing is public API, but looking at AsComponent it indeed is...

Fixed that one too, I wonder why I don't get a compilation error here while CI obviously reports that.

Configure AppStream with -DMAINTAINER=ON, that will enable (almost) all compile-time maintainer checks.

Probably useful to do that locally, as the CI still isn't happy :P

CI is now happy, want me to squeeze all these commit into one or later on once you are happy about that?

ximion · 2017-06-16T19:19:30Z

Looks good to me! Can you maybe fold in the two return-fix commits, and prefix the changes with "qt"?

I am not really happy about exposing the raw GObjects in the Qt bindings in a public interface, but we can find a solution for that after merge, if there is one.

aleixpol

LGTM in general. Just nitpicking.

aleixpol · 2017-06-16T23:57:06Z

qt/contentrating.cpp

+
+AppStream::ContentRating::RatingValue AppStream::ContentRating::stringToRatingValue(const QString& ratingValue)
+{
+    if (ratingValue == QLatin1String("none")) {


Why not a hash here?

Fixed to use hash even here.

aleixpol · 2017-06-16T23:57:23Z

qt/contentrating.cpp

+    AsContentRating* m_contentRating;
+};
+
+typedef QHash<ContentRating::RatingValue, QString> RatingMap;


A vector should be enough?

Not sure if this would make any difference, QHash is used accross appstream same way so I used it here as well.

What's the overhead of a QHash? Would using a vector be much better? If not, keeping the QHash is fine, I guess.

aleixpol · 2017-06-16T23:58:37Z

qt/launchable.cpp

+#include "appstream.h"
+#include "launchable.h"
+
+#include <QDebug>


include possibly not needed?

Taking back, it's needed.

Also implement missing classes like AS::Launchable, AS:Translation or AS:ContentRating.

ximion · 2017-06-19T15:47:37Z

Okay, merged - it's one massive commit now, maybe breaking this down would have been nicer, but well, too late...

Thank you for your patch!

ximion reviewed Jun 14, 2017

View reviewed changes

grulja force-pushed the master branch from 0379340 to ed53e6b Compare June 15, 2017 11:50

ximion reviewed Jun 16, 2017

View reviewed changes

aleixpol reviewed Jun 16, 2017

View reviewed changes

Add missing getters/setters for most of the properties in AS::Component

5e58a60

Also implement missing classes like AS::Launchable, AS:Translation or AS:ContentRating.

grulja force-pushed the master branch from abc80bd to 5e58a60 Compare June 19, 2017 05:46

ximion merged commit 548a5ae into ximion:master Jun 19, 2017

Comments

Conversation

grulja commented Jun 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ximion commented Jun 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ximion commented Jun 16, 2017

Uh oh!

aleixpol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ximion commented Jun 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants