Opened 5 years ago

Closed 9 months ago

Last modified 9 months ago

#11614 closed enhancement (fixed)

wxFontPickerCtrl doesn't allow to set nor retrieve the font colour

Reported by: fniessink Owned by:
Priority: normal Milestone:
Component: GUI-all Version:
Keywords: font picker simple API Cc:
Blocked By: Blocking:
Patch: no

Description

wx.FontPickerCtrl allows users to set the font color, but there is no way to access the color chosen by the user.

Attachments (2)

font_picker_ctrl.patch download (8.9 KB) - added by astronothing 9 months ago.
initial implementation
font_picker_ctrl_revised.patch download (7.0 KB) - added by astronothing 9 months ago.
revised implementation

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 years ago by vadz

  • Keywords picker simple API added
  • Status changed from new to confirmed
  • Summary changed from No access to font color set via wx.FontPickerCtrl to wxFontPickerCtrl doesn't allow to set nor retrieve the font colour
  • Type changed from defect to enhancement

Indeed, this is an unfortunate omission. I don't think we need the entire wxFontData contents here but it'd be definitely nice to have Set/GetColour() methods in this class.

comment:2 Changed 9 months ago by astronothing

  • Status changed from confirmed to infoneeded_new

After investigating this, it appears that font color is not available for every platform. The GTK2 font picker dialog for instance does not offer the choice of a color. We are thus faced with three possibilities:

  1. Implement adequate font color retrieval for supported systems (msw) and return a default color on every other system.
  2. Add access to wxFontData from a wxFontPickerCtrl.
  3. Ignore this feature.

What would be the desired choice?

comment:3 Changed 9 months ago by vadz

  • Status changed from infoneeded_new to new

True, this is MSW-specific, but this is the same for wxFontDialog and we do provide access to the colour there, so why not here as well. I.e. I still think the best would be to provide {Set,Get}Colour() methods and just document clearly that the colour can only be actually changed by user under MSW.

Changed 9 months ago by astronothing

initial implementation

comment:4 Changed 9 months ago by astronothing

I've attached a patch with an initial implementation. I've also taken the liberty of replacing a #define with a private member function and replacing an old style C cast with a static_cast.

I've added two virtual methods to wxFontPickerWidgetBase which is used to represent the control that displays the font dialog. They allow the setting and getting of the font colour.

For wxMSW these are implemented through the wxFontData available in wxGenericFontButton.
For wxGTK2 these are implemented by using a new data member.

A simple tests case is also included.

comment:5 Changed 9 months ago by vadz

Thanks!

This is broadly good but a few remarks before it can be applied without changes:

  1. The new methods must be documented in the corresponding documentation file. Please use @since 3.1.0 in the methods documentation to indicate that they are indeed new.
  2. Adding tests is an excellent idea, thanks! But it would be better to use just compare the colours directly, without using .GetRGB(), we already have an overloaded operator<< for wxColour which would print it out nicely.
  3. There is some suspicious indentation in the patch, please check for the absence of hard TABs in the sources.
  4. Another stylistic issue: getPickerWidget() doesn't follow wxWidgets CamelCase() function naming convention.
  5. Fixing typos is appreciated but generally speaking, please try keep separate changes in separate patches/commits.
  6. There is no need to update the "Modified by" header (which should really be globally removed as it's become obsolete since the advent of version control systems sometimes in the last millennium), you will be credited for your changes in the change log.

TIA!

comment:6 follow-up: Changed 9 months ago by astronothing

  1. I'm sorry I didn't take the time to find the documentation header. I'll make sure the documentation is up to date from now on.
  2. I started off by comparing the two colors, but my compiler threw an error because of a missing operator<< so I used the .GetRGB method instead.
  3. You might be right, I only removed tabs after finishing the implementation. I'll configure my editor accordingly next time.
  4. That slipped, sorry.
  5. Unfortunately, I love fixing typos. Could I perhaps make a new ticket and submit a larger patch there ?
  6. I was inspired by a recent patch: http://trac.wxwidgets.org/ticket/11815 but I'm happy to hear it's no longer required. Perhaps open a ticket requiring the removal of such artifacts?

Thank you for your comments, I'll submit an updated patch shortly.

comment:7 in reply to: ↑ 6 Changed 9 months ago by vadz

Replying to astronothing:

  1. I started off by comparing the two colors, but my compiler threw an error because of a missing operator<< so I used the .GetRGB method instead.

You need to #include "asserthelper.h" where this operator is declared.

  1. Unfortunately, I love fixing typos.

Excellent :-) There is nothing wrong with doing this, it's just that it's a bad idea to intermix significant changes with cosmetic ones (or, more generally speaking, any kind of unrelated changes). So definitely feel free to submit the typo fixes as a separate patch and I'd gladly commit it.

  1. I was inspired by a recent patch: http://trac.wxwidgets.org/ticket/11815

It was actually by another student and I was just too lazy to mention it there as it was simpler to just drop this part of the patch, as there was only a single hunk with this change. In your patch, there are several of them so I decided to mention it instead...

but I'm happy to hear it's no longer required. Perhaps open a ticket requiring the removal of such artifacts?

It's not really urgent, but yes, we could do that.

Thank you for your comments, I'll submit an updated patch shortly.

TIA!

Changed 9 months ago by astronothing

revised implementation

comment:8 Changed 9 months ago by astronothing

I've attached a revised patched. Hopefully nothing else slipped this time.

comment:9 Changed 9 months ago by vadz

Looks good to me, thanks!

Will commit soon.

comment:10 Changed 9 months ago by vadz

P.S. Having unit tests is great, but they must also pass! And this wasn't the case under GTK. I've fixed this (see the code I'm committing), but please do actually run the tests and do it under as many platforms as possible. TIA!

comment:11 Changed 9 months ago by VZ

(In [76159]) Use helper GetPickerWidget() function in wxFontPickerCtrl.

No real changes, just use a helper function instead of an ugly M_PICKER macro.

See #11614.

comment:12 Changed 9 months ago by VZ

  • Resolution set to fixed
  • Status changed from new to closed

(In [76160]) Add font colour support to wxFontPickerCtrl.

Currently this is only really implemented under Windows, just as the colour
support in wxFontDialog, but make the API available under all platforms for
consistency.

Closes #11614.

comment:13 Changed 9 months ago by astronothing

I noticed you separated the helper function from the actual implementation. It's good to know that individual commits should be kept consistent to their purpose. I'll keep that in mind.

Also, I apologize for not running the tests under GTK. Unfortunately, I can only run them under Linux and Windows (which I will) but I will not be able to run them under MacOS.

Thank you for the integration :)

Note: See TracTickets for help on using tickets.