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

Add two new expression functions: rgb_to_hex, to_base #39655

Closed
wants to merge 1 commit into from
Closed

Add two new expression functions: rgb_to_hex, to_base #39655

wants to merge 1 commit into from

Conversation

hamiltoncj
Copy link
Contributor

Description

[FEATURE] This adds two new expression functions:

Function rgb_to_hex - Returns a hex string representation of a color based on its red, green, blue, and optional alpha components. The output is either '#RRGGBB' or '#RRGGBBAA' where RR, GG, BB, and AA are the red, green, blue and alpha components.

Function to_base - Convert an integer value to a string representation in a different base or number system. Binary is base 2, octal base 8, and hexadecimal base 16.

rgb_to_hex

to_base

These are both function that I have found missing and wanted them for my use. Since this is my first code commit I only worked on these two, but plan on other related functions.

@github-actions github-actions bot added this to the 3.18.0 milestone Oct 27, 2020
@luipir luipir changed the title Add two new expression functions: gb_to_hex, to_base Add two new expression functions: rgb_to_hex, to_base Oct 29, 2020
@nyalldawson
Copy link
Collaborator

@hamiltoncj Code looks good -- nice work!

I'll leave the discussion about the formatting scripts to the mailing list thread in order to keep things centralized. Apart from that, the only other change required here is some new tests for the new functions.

The color ones would sit somewhere around here:
https://github.com/qgis/QGIS/blob/master/tests/src/core/testqgsexpression.cpp#L1490

each line looks like this:

QTest::newRow( "ramp color" ) << "ramp_color('Spectral',0.3)" << false << QVariant( "254,190,116,255" );

where the "ramp color" string is a unique name identifying the particular test. It should have enough detail to identify exactly which tests fails, so a good name is something like "rgb_to_hex red greater than 255".
The second argument ("ramp_color('....')) is the expression to test. The next "false" is whether the expression is invalid and should raise an error (e.g. when an input is invalid), and finally the last is the expected result from the expression.

You add as many lines as necessary to give your functions a good work out. Include ones which test the function with bad inputs, e.g. string values and components <0 or > 255.

The to_base tests could go somewhere around here: https://github.com/qgis/QGIS/blob/master/tests/src/core/testqgsexpression.cpp#L838

@nyalldawson
Copy link
Collaborator

BTW -- welcome to the wonderful world of core QGIS development! I realise it's a steep learning curve, but after you get past this I'm sure you'll find it's a pleasure to contribute and the possibilities your enhancements can take will really explode. It's great to have you onboard!

@hamiltoncj
Copy link
Contributor Author

So I am not sure about the process of correcting the styling given that I am currently doing this on windows and once I correct it how do I update the pull request code? Sorry for not knowing this, but this is completely new to me.

@nyalldawson
Copy link
Collaborator

@hamiltoncj I've replied via the qgis Dev mailing list thread, can we keep the conversation centred there?

@alexbruy
Copy link
Contributor

alexbruy commented Nov 1, 2020

There is related ticket #23988 with good example how to extract color components using existing functions.

@stale
Copy link

stale bot commented Nov 21, 2020

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@stale stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 21, 2020
@nyalldawson
Copy link
Collaborator

Closing manually because stale-bot seems to have gone on holiday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Uh oh! Seems this work is abandoned, and the PR is about to close.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants