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

Feature: Add "Remove tab colour" method #1199

Merged
merged 19 commits into from
Jun 8, 2023
Merged

Feature: Add "Remove tab colour" method #1199

merged 19 commits into from
Jun 8, 2023

Conversation

alifeee
Copy link
Collaborator

@alifeee alifeee commented May 31, 2023

Closes #1166

Google colours

This ought to have been "simple", but it was a little more complex. Here are two tabs in a Google sheet:

image

The left is #ff007f, and the right is unset.

Google returns the tabColorStyle for each as:

{
  "sheets": [
    {
      "properties": {
        "sheetId": 0,
        "title": "Data",
        "tabColor": {
          "red": 1,
          "blue": 0.49803922
        },
        "tabColorStyle": {
          "rgbColor": {
            "red": 1,
            "blue": 0.49803922
          }
        }
      }
    },
    {
      "properties": {
        "sheetId": 90870515,
        "title": "Information"
      }
    }
  ]
}

Thus, the returned colours do not include 0 values (i.e., there is no "green" in the rgbColor dict), and they are rounded to the nearest 8-bit decimal between 0 and 1 (i.e., "blue" is 127/255, or the result of setting "blue" to 0.5 with gspread).

Code to generate this output:

import gspread
import json

gc = gspread.service_account(filename="./creds.json")
worksheet = gc.open("gspread test spreadsheet")

params = {
    "fields": "sheets.properties.sheetId,sheets.properties.title,sheets.properties.tabColor,sheets.properties.tabColorStyle",
}
properties = worksheet.fetch_sheet_metadata(params=params)
with open("properties.json", "w", encoding="utf-8") as f:
    json.dump(properties, f, indent=2)

Changes

Worksheet.tab_color

  • Expand docstring to explain the result you may get from Google's tab colour

Worksheet.update_tab_color

  • Add type hint (dict)
  • Remove "tabColor" (deprecated, see Add ability to remove tab color #1166)
  • Method no longer sets `self._properties["tabColorStyle"] to color given by user
    • This is because of the above. The colour in Worksheet._properties would be different to the one returned by Spreadsheet.fetch_sheet_metadata().
    • The colour is fetched from the spreadsheet and used to update the internal _properties value. *
  • Update test_update_tab_color to reflect this change
  • Update test cassette WorksheetTest.test_update_tab_color.json

*this may be an unwanted extra API request

Worksheet.clear_tab_color

  • Add new method clear_tab_color
  • Add test test_clear_tab_color
  • Add test cassette WorksheetTest.test_clear_tab_color.json

Docs

Add "Updating a Worksheet's name and color" to "Examples of gspread Usage"

@lavigne958
Copy link
Collaborator

Hi thank you for this contribution.

I need some time to look at it.

It looks nice at first glance. 👍

Copy link
Collaborator

@lavigne958 lavigne958 left a comment

Choose a reason for hiding this comment

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

This is very interesting. thank you for catching it !

In gspread, users tend to reach their quotas very quickly, in this regard we try to minimize the number of API calls.

Do you instead of the getting the value back from the API (value that we just set) we could compute it locally instead ?

this is less reliable but should get us the same result and prevent us from having new API calls.

eg:

  • we could round to the nearest lower positive integer value then divide by 255 which should give us the same value
    like:
green=0.5
real_green = rount(math.floor(green*255) / 255)
green
>>> 0.49803922

the above value is exactly what I get from the API but using computation only.

what do you think ?

@alifeee
Copy link
Collaborator Author

alifeee commented Jun 6, 2023

Minimising API requests is always nice.

The below assumes that strict equality between colours is required (e.g., if a user wants to use "==" to compare colors)

For this function, the problem is the rounding. Google returns colors inconsistently rounded:

image

We could:

  • compute colours manually, and they will not be exactly the same as Google, but near enough (if this is done, there is not much point manually computing the coalescence, since the user input "0.5" will be just as "close enough" as our badly rounded attempt)
  • keep the API call

@alifeee alifeee marked this pull request as draft June 6, 2023 22:31
@lavigne958
Copy link
Collaborator

Yeah I see 🧐 I don't think the user will compare so much the colors but as you mentioned: if someone wants to check the current color then they will read "0.495857" and so it won't latch so they will set it to "0.5" which will have not effect because it's already set and cost 1 API call 😬

Thinking about it:

  • the API uses float values
  • but colors are always set using integer between [0:255]
  • why do we have to ask the user for a float and then change it 🧐

Can't we use whol numbers from 0 to 255 do the computation for the API and then return the appropriate value for the user ?

You see what I mean?

Btw: that's a breaking change 😛

@alifeee
Copy link
Collaborator Author

alifeee commented Jun 7, 2023

Yes, this sounds good.

If the color is set with 0-255 (i.e., with hex #RRGGBB), then it makes sense to use that to set/read color, rather than these 'random' floats.

Then, we could also offer either

  • set_color("#FF0080")
  • set_color(255, 0, 128)
  • set_color({"red": 255, "green": 0, "blue": 128})

Not sure which you would prefer. You likely have a good idea from your signature typing adventure which is easiest/best.

Let's put this pull request in milestone v6.0.0?

@lavigne958
Copy link
Collaborator

Yes, this sounds good.

If the color is set with 0-255 (i.e., with hex #RRGGBB), then it makes sense to use that to set/read color, rather than these 'random' floats.

Then, we could also offer either

  • set_color("#FF0080")
  • set_color(255, 0, 128)
  • set_color({"red": 255, "green": 0, "blue": 128})

Not sure which you would prefer. You likely have a good idea from your signature typing adventure which is easiest/best.

Let's put this pull request in milestone v6.0.0?

I was gonna say the same thing: this becomes a breaking change that we will put in 6.0.0 😆

currently we can keep the PR with:

  • we remove the old field that we don't need to set anymore tabColorStyle, as it has been deprecated.
  • we add the new method to remove the tab color, that is still useful for users
  • we keep the way we set the colors in the properties: this way if a user sets a color of red: 0.5 when the user reads the value it gets: 0.5 what has been set previously. no fuzzing with float values.

the rest we can add later:

  1. a new method to set the color of a tab, something like: set_tab_color(hex_color) that takes only a color as hex value
  2. a new function in utils.py that can convert a color from 3 arguments to a hex value: convert_colors_to_hex_value(red, gree, blue) this way a user can use it 2 in ways seamlessly:
    1. using direct argument assignment: ..._hex_value(123,43,12)
    2. using named arguments: ..._hex_value(red=123, green=43, blue=12)

@alifeee
Copy link
Collaborator Author

alifeee commented Jun 7, 2023

Sounds good.

I can do the first step now to edit the PR.

For the additions: I am not that good at function decorators (so they can take multiple arguments). Perhaps something for you to look at.

Should we make an issue for it?

just put the user input colour into the properties dict
this will not equate properly, but
see #1199 for discussion
@alifeee alifeee marked this pull request as ready for review June 7, 2023 23:46
@alifeee alifeee requested a review from lavigne958 June 7, 2023 23:46
@alifeee
Copy link
Collaborator Author

alifeee commented Jun 7, 2023

I will be happy to merge this as it is the last branch left in my fork ^^

Copy link
Collaborator

@lavigne958 lavigne958 left a comment

Choose a reason for hiding this comment

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

Looks good to me, merge time.

@alifeee alifeee merged commit fde332b into burnash:master Jun 8, 2023
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.

Add ability to remove tab color
2 participants