Opened 6 years ago

Closed 3 years ago

Last modified 15 months ago

#9907 closed enhancement (fixed)

Add font strikethrough support for (with patch for wxMSW)

Reported by: ConfusedSushi Owned by:
Priority: low Milestone: 2.9.4
Component: GUI-all Version: stable-latest
Keywords: wxFont strikethrough MSW GTK Cc:
Blocked By: Blocking:
Patch: yes

Description

The documentation says there is a flag for setting a striked font.

wxFONTFLAG_STRIKETHROUGH

But passing the flag to wxFont::New has currently no effect.

A patch is attached which implements this feature for MSW.

Also all small test application is attached in fonttest.cpp.

The following public methods have been added/changed:

void wxFont::SetStrikethrough(bool)

bool wxFont::GetStrikethrough()

wxFont::wxFont(int pointSize, wxFontFamily family, int flags, const wxString& face, wxFontEncoding encoding)

`static wxFont * New(const wxSize& pixelSize, wxFontFamily
family, int flags = wxFONTFLAG_DEFAULT, const wxString& faceName =
"", wxFontEncoding encoding = wxFONTENCODING_DEFAULT)`

Please note that SetStrikethrough and GetStrikethrough are pure virtual and may should be removed to get a usable source on platforms other than MSW.

I would be happy to see that in your next release.

Please let me know if you have any suggestions.

Greetings

Marcel

Attachments (5)

wx-font-strikethrough.patch download (11.8 KB) - added by ConfusedSushi 6 years ago.
fonttest.cpp download (6.1 KB) - added by ConfusedSushi 6 years ago.
simple app to demonstrate the pach (runs with and without)
wx-font-strikethrough-v2.patch download (15.3 KB) - added by ConfusedSushi 6 years ago.
strikethrough.2.patch download (31.7 KB) - added by oneeyeman 3 years ago.
Patch version 3
strikethrough.patch download (39.3 KB) - added by oneeyeman 3 years ago.
Implementation for MSW and GTK

Download all attachments as: .zip

Change History (27)

Changed 6 years ago by ConfusedSushi

Changed 6 years ago by ConfusedSushi

simple app to demonstrate the pach (runs with and without)

comment:1 Changed 6 years ago by vadz

  • Keywords wxFont added
  • Status changed from new to confirmed
  • Summary changed from Font Strikethrough support to Font Strikethrough support for wxMSW

Thank you for your patch, especially updating the docs!

However it can't be applied quite "as is" because this would break all the other ports compilation (their wxFont implementations not defining the new pure virtual functions). The simplest fix is to provide stubs (always returning false) instead of making the functions pure virtual.

Also, submitting a test application demonstrating a new feature is an excellent idea, but we already have one which is almost like this: the font sample. Would it be possible to update it to show strike through fonts?

TIA!

comment:2 Changed 6 years ago by ConfusedSushi

Hello,

here is the updated patch.

What I have done:

  • Updated the font sample provided by wx
  • Updated the MSW code behind GetNativeFontInfoUserDesc/SetNativeFontInfoUserDesc to be aware of a new token "strikethrough"
  • SetStrikethrough/GetStrikethrough now are providing a default implementation doing nothing and returning false respectively

Changed 6 years ago by ConfusedSushi

comment:3 Changed 6 years ago by wojdyr

The new wxFont ctor can be confusing, because it's very similar to one that's already there.
IMO wxFont has enough (or too many) constructors and adding another one is not a good idea.

comment:4 follow-up: Changed 6 years ago by ConfusedSushi

I agree with you, wxFont has many c'tors, but how could it be done else?

I think there are three options:

  1. Leave it as it would become with the patch, the new c'tor is reasonable, creating a new font with all aviable options through the flag parameter is a straight forward way, may the c'tors with the boolean parameter underlined could be removed, but this would break backward compatiblity
  2. Change the allready aviable c'tors by adding a new boolean parameter, but this would break backward compatiblity and conflicts with the coding rules (avoid boolean parameters)
  3. Make the new c'tor private or protected, since it is accessible through a static New method, no functionality get lost, but this would introduce an additional, IMO unnecessary, function call, which might have an impact on performance (I have to create many different fonts quickly)

To me option one should be preferred, but therefor additional discussion is needed.

For now option three would be the easiest solution.

Let me know what you think.

Greetings

Marcel

comment:5 in reply to: ↑ 4 Changed 6 years ago by vadz

  • Status changed from confirmed to infoneeded_new
  • Summary changed from Font Strikethrough support for wxMSW to Font strikethrough support for wxMSW

Replying to ConfusedSushi:

I agree with you, wxFont has many c'tors, but how could it be done else?

I think there are three options:

  1. Leave it as it would become with the patch, the new c'tor is reasonable, creating a new font with all aviable options through the flag parameter is a straight forward way, may the c'tors with the boolean parameter underlined could be removed, but this would break backward compatiblity

I do like the idea of the ctor taking flags, this is why I had added the flags and static New() method taking them in the first place. But the problem I have with the patch is that it only adds the new ctor for wxMSW, I'd like to have it for the other ports too.

And I'd also like to change DoCreate() in wx/msw/font.h to take flags instead of adding yet another parameter to it (9 is more than enough), i.e. making the flags-based code the primary code path and keep the other ctors as synonyms.

Would it be possible for you to update the patch to do this? Otherwise it's almost perfect.

Thanks in advance!

comment:6 Changed 5 years ago by ConfusedSushi

Because a lack of time, for now I can't work on this.
May someone else would provide this feature for other ports.

comment:7 Changed 5 years ago by vadz

  • Patch unset
  • Status changed from infoneeded_new to new
  • Summary changed from Font strikethrough support for wxMSW to Add font strikethrough support for (with patch for wxMSW)

Ok, it still can't be applied in the current state but maybe indeed someone else can implement it for the other ports and also improve the API to avoid too long parameter lists.

comment:8 Changed 5 years ago by vadz

  • Priority changed from normal to low
  • Status changed from new to confirmed

comment:9 Changed 3 years ago by oneeyeman

  • Patch set

Hi, ALL,
I finally have time to work on it.
Attached please find the patch that implements this feature for MSW and GTK+. I would appreciate if Vadim, Robert or Paul review the GTK part and review the patch as a whole.

Thank you.

comment:10 Changed 3 years ago by oneeyeman

  • Keywords strikethrough MSW GTK added

comment:11 Changed 3 years ago by vadz

This patch is mostly very good and while I won't mention everything it does correctly, I do appreciate the efforts that went into it, thanks a lot!

Unfortunately there are still some serious problems which prevent it from being applied:

  1. The main one: the change of wxFont ctor signature breaks backwards compatibility and this just can't be done because there is plenty of code creating wxFonts and there is just no good reason to break it all. I don't understand why did this change, too, as the initial versions of the patch added a new ctor taking flags instead of modifying the existing one and I still very much prefer this approach. If we want to specify this attribute in the ctor we really ought to add a new one with the same signature as flag-taking New() method.
  2. This is somewhat related to the previous point but AFAICS this patch still breaks compilation of wxOSX and all the other non-{MSW,GTK} ports. It is, of course, perfectly acceptable to not have support for strike-through in these ports but the existing code should still continue to compile which is not the case because the existing ctor was modified. But even if a new ctor is added, it would still need to be done for all ports.
  3. The serialization format version must be changed as the new format is incompatible with the old one. Ideally we should continue generating version 0 output if strike-through is not used and switch to version 1, with an extra field, if it is set only to remain maximally compatible. Of course, we should in any case be able to read both version 0 and 1 of the format.

A few other, much less serious, comments:

  1. It'd be nice to have MakeStrikethrough() and Strikethrough() methods similar to the existing MakeBold/Bold() and other pairs.
  2. The documentation of everything strike-through related should have @since 2.9.2 in it.
  3. XRC format specification in docs/doxygen/overviews/xrc_format.h needs to be updated to mention the new attribute.

As for GTK code, it seems reasonably straightforward to me but I'm not an expert, of course. I do wonder if we could avoid some of the repetitions of the same code by extracting it into a common helper function somehow?

To summarize, I wonder if we could perhaps apply the patch without any changes to the ctors and Create() methods at all but instead just add SetStrikethrough() (and maybe the related MakeStrikethrough())? This wouldn't allow to create strike-through fonts directly but it would at least allow creating them somehow... Of course, implementing the new ctor in all ports would be even better but this will require more work.

comment:12 Changed 3 years ago by oneeyeman

Hi, Vadim,
Thank you for taking the time and reviewing this patch. It is very much appreciated.
Unfortunately I wasn't able to reply earlier, so here goes:

  1. Yes, I thought about this one. While this breakage is serious it allowed me to pinpoint all places where the font construction was used and so when we get to the consensus about best fix for that all those places will be taken care of. Let me assure you that I did read the code submitted by my predecessor and all the comments here. Now lets talk about the best approach of doing it. What would you think would be best? Maybe put the new parameter at the end of the constructor? Create another wxFont constructor with all possible parameters? The font creation code (constructors/static New) is aleady bloated.
  1. When I tried to compile wxUniv on MSW it compiled fine. I can't speak for wxMac as I can't test this at all, but if you say it will then some changes will have to be done based on the decision of 1 to Mac port. Unfortunately I don't have Mac and even if I did know even less about it than about GTK+/Pango. So whoever will check it will have to make sure that different Mac ports will still works.
  1. I think it might be also related to the point 1 above. Because as you can see I implemented serialization based on the constructor approach. However, I agree that we should continue to support (de)-serializing old format of wxFont, which means it will be more learning for me. ;-) Let me ask you here: does wx support different version (de)-serializing? If yes, where should I look for proper implementation?

Now, let me address other issues:

  1. I will look at that possibility.
  2. Will add in the next version of the patch.
  3. Thank you for pointing that out.

About GTK. When I ran the sample and selected the "Strikethrough" menu, the font changed, but for some odd reason only in the lower half of the window. The text in the top text control didn't change. However, I feel that there is something missing in the sample itself, hence the request of the code review from Robert or Paul.

I am not afraid of more work. I would actually implement everything properly from the very beginning than come back later on and change/fix whatever needs to be done/fixed.

If you prefer to continue the constructor-related discussion on wx-dev, just say so.

Changed 3 years ago by oneeyeman

Patch version 3

comment:13 Changed 3 years ago by oneeyeman

  • Component changed from wxMSW to GUI-all
  • Milestone set to 2.9.4

Hi,
As suggested by Vadim in his last comment this patch implements strikethrough font reation only.
It also implements a possibility to save the font with the new feature and restore it with both old and new structure.

It does not touch neither old constructor code nor any other libraries but core.
The patch contains documentation for a new feature. If it will bwe applied to 2.9.3 then @since tag needs to be changed. Also documentation needs to reflect the fact that the implementation is currently for MSW and GTK+. And the documentation needs to be reviewed by native English speaker of the community.

The next patch will refactor the code so it will be possible to construct the strikethrough font and not just create it.

All comments are welcome as usual.

Thank you.

comment:14 Changed 3 years ago by oneeyeman

Sorry forgot to mention.
The issue on GTK+ still exists. Only text in the bottom part of the window becomes striken.

I will try to debug it but in the meantime if someone familiar with GTK+ can give a clue, it would be appreciated.

Thank you.

comment:15 Changed 3 years ago by oneeyeman

OK, the problem is fixed.
I'm attaching the final version of the patch that implements strikethrough font for both MSW and GTK+. It is ready to be applied after the review.
Once again, I'd aprreciate the documentation review from native English speaker. ;-)
The patch is based on the wx SVN TRUNK revision 69876.

Thank you.

Changed 3 years ago by oneeyeman

Implementation for MSW and GTK

comment:16 Changed 3 years ago by oneeyeman

Also want to mention that this patch takes care of the wxUniversal port on Windows. I didn't test anything else.

comment:17 Changed 3 years ago by vadz

Thanks, I've finally applied this but it took me almost 2 hours to deal with it again :-( Here is my usual list of changes:

  1. Fixed indentation: while it's nice to not have TABs in the patch, it's not nice that the indentation is still broken, just look at how your patch appears if you click on it above...
  2. Added the new wxFont(... int flags ...) ctor to all ports. It's useless to have it in only wxMSW and wxGTK as this prevents its use in portable code. So even if it does nothing more than the existing ctors in the other ports you still must have it there.
  3. Actually implement this ctor in wxMSW and wxGTK -- how did you manage to add its declaration but not its definition?? How exactly did you expect this to work?
  4. Renamed the confusingly named CheckUnderlineOrStrikethrough(): what does "check" mean here? I called it SetPangoAttrsForFont() which is IMO much more clear. I also documented it in a comment as its semantics is not obvious. And added a return value to it as your code didn't update the tests before restoring the attributes everywhere and couldn't do it without this return value.
  5. Removed the commented out code that the calls to the above function replaced. Please don't leave the old code in your patches, this makes it much harder to see what has changed and, of course, the old code still needs to be removed before committing.
  6. Fixed the logic in wxStaticText::SetFont() which was broken in too many ways to count (you can't always just copy the existing code and expect it to continue to work...).

comment:18 Changed 3 years ago by VZ

(In [70445]) Add wxFont ctor taking a single flags argument instead of style/weight/...

Currently this ctor just does the same thing as the existing ctors in a
different way but it will be extended to support wxFONTFLAG_STRIKETHROUGH in
the next commits.

See #9907.

comment:19 Changed 3 years ago by VZ

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

(In [70446]) Add support for stricken-through fonts.

Support stricken-through fonts in wxMSW and wxGTK (including special support
in wxStaticText and wxTextCtrl).

Closes #9907.

comment:20 Changed 3 years ago by VZ

(In [70447]) Implement support for stricken-through fonts in markup parser.

Now that we have strike-through support in wxFont, implement support for <s>
tag in the markup parser.

See #9907.

comment:21 Changed 3 years ago by VZ

(In [70448]) Fix wxTextAttr::m_fontStrikethrough initialization.

The code added in r70447 didn't initialize m_fontStrikethrough correctly.

This fixes unit test failures for wxRichTextCtrl that appeared since this
change.

See #9907.

comment:22 Changed 15 months ago by VZ

(In [73885]) Add wxFontInfo class to allow using named parameters for wxFont creation.

This helper class allows to create wxFonts using shorter and more readable
code, e.g.

wxFont font(12, wxFONTFLAG_DEFAULT,

wxFONTSTYLE_NORMAL, wxFONTWEIGHT_NORMAL, true,
"DejaVu Sans");

can now be written simply as

wxFont font(wxFontInfo(12).FaceName("DejaVu Sans").Underlined());

Remove the ctor from font flags added in r70445 as it's not needed any longer
now that we have this one and adding it resulted in compilation errors in the
existing code which compiled with 2.8 because of ambiguities between that ctor
and wxFont(int size, int family, int style, int weight. bool underlined, ...)
one, e.g.

wxFont(12, wxFONTFAMILY_SWISS, wxNORMAL, wxNORMAL)

didn't compile any more but it does compile again now.

See #9907.

Note: See TracTickets for help on using tickets.