Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for C++ theming components #2

Merged
merged 3 commits into from Oct 7, 2013
Merged

Added support for C++ theming components #2

merged 3 commits into from Oct 7, 2013

Conversation

SfietKonstantin
Copy link
Member

No description provided.

@@ -1,19 +1,19 @@
<!DOCTYPE RCC><RCC version="1.0">
Copy link
Member

Choose a reason for hiding this comment

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

why did you delete this? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

QtCreator did it :(

This commit imports work done for having C++ based
theming, exporting a C++ Theme singleton object,
and C++ object for each component, starting with
the button.

It also includes the new theming system, using JSON,
and a tool to generate new components based on a
description file.
@faenil
Copy link
Member

faenil commented Oct 6, 2013

it all looks very good to me, except the part about inner objects, like NemoThemePressedGradient...

it would be good to have nested objects in the components.json definitions as well...

Of course this requires a bigger effort while parsing, but it looks a bit dirty to me as it is now :( having button and its inner properties on the same level...
It's more of an implementation thing, as the API from QML is more or less the same, but I think that if we don't change it now, nobody will change it in the near future

Also, I'm no python guy (yet :D ) so I can't comment on the code generator, but I suggested few c++ related fixes (such as the use of const for setters)

That said, I think you made a very good job with all the rest :)
The final result will be awesome ;)

source += "\n"

if "type" in property:
source += "void " + name + "::set" + _getUpper(propertyName)
Copy link
Member

Choose a reason for hiding this comment

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

could it be worth adding a "const" here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Look at the generated code: the consts are here if needed.

Copy link
Member

Choose a reason for hiding this comment

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

doesn't look like that :P

Copy link
Member Author

Choose a reason for hiding this comment

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

Now it is ok :P

@locusf
Copy link
Member

locusf commented Oct 7, 2013

LGTM

@SfietKonstantin
Copy link
Member Author

LGTF: Look good to Faenil :)

SfietKonstantin added a commit that referenced this pull request Oct 7, 2013
Added support for C++ theming components
@SfietKonstantin SfietKonstantin merged commit 4ca9987 into nemomobile:master Oct 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants