Opened 2 years ago

Closed 2 years ago

#14706 closed optimization (fixed)

Choice changes for wxTextMeasure

Reported by: mmarsan Owned by:
Priority: low Milestone:
Component: GUI-all Version: stable-latest
Keywords: choice combo textextent Cc:
Blocked By: #14705 Blocking:
Patch: yes

Description

This patch starts the use of wxTextMeasure::GetLargestStringExtent()
It requires the patch in ticket 14705.

Changes are in GetBestSize() of src/msw/choice.cpp and src/gtk/choice.cpp

This code can be also easily copied to other (i.e. OX) choice and combobox implementations.

Attachments (4)

textmeasure_choice.patch download (3.0 KB) - added by mmarsan 2 years ago.
textmeasure_choice_v2.patch download (3.2 KB) - added by mmarsan 2 years ago.
Replaces the other
textmeasure_choice_v3.patch download (5.7 KB) - added by mmarsan 2 years ago.
Gather code in wxChoiceBase instead of msw/gtk wxChoice versions
textmeasure_choice_v4.patch download (4.6 KB) - added by mmarsan 2 years ago.

Download all attachments as: .zip

Change History (20)

Changed 2 years ago by mmarsan

comment:1 Changed 2 years ago by vadz

  • Blocked By 14705 added

comment:2 Changed 2 years ago by mmarsan

  • Blocked By

(In #14705) I attach the version 2 (replaces the other)

comment:3 Changed 2 years ago by mmarsan

I attach the new version (v2) reworked for v2 of patch in ticket 14705

Changed 2 years ago by mmarsan

Replaces the other

comment:4 Changed 2 years ago by vadz

  • Blocked By

(In #14705) Just FYI: thanks for the update, I'm working on applying this.

comment:5 Changed 2 years ago by VZ

  • Blocked By

(In #14705) (In [72699]) Factor out text measurement from wxDC and wxWindow into wxTextMeasure.

Add a new private wxTextMeasure class implementing methods for measuring text
and move the often duplicated (but not always identically) code for doing the
same from wxDC and wxWindow into it.

Currently this class is only really implemented in wxMSW and wxGTK.

Also extend the test for text measuring functions and rename it to
MeasuringTextTestCase from MeasuringContextTestCase as it's not wxGC-specific
any more.

Closes #14705.

comment:5 Changed 2 years ago by vadz

  • Blocked By
  • Priority changed from normal to low

The code is so similar in the 2 ports that it would be really better to put it into wxChoiceBase. You could use #if !wxUSE_GENERIC_TEXTMEASURE to do it only for the ports that actually have wxTextMeasure or just do it unconditionally, AFAICS it shouldn't do any lasting harm neither.

Could you please update the patch to do it at the base class level? TIA!

Changed 2 years ago by mmarsan

Gather code in wxChoiceBase instead of msw/gtk wxChoice versions

comment:6 Changed 2 years ago by mmarsan

I've attached the version 3, replaces the others.

comment:7 Changed 2 years ago by vadz

Thanks, but it's still unfortunately not perfect. Here is what I'd like to see improved:

  1. Provide wxChoiceBase::DoGetBestSize() for all ports. As wxTextMeasure can now be used everywhere (by just forwarding back to wxDC if not implemented natively), I don't see any reason to explicitly test for __WXMSW__ || __WXGTK__ here (and if such reason does exist, it would still be better to use #if !wxUSE_GENERIC_TEXTMEASURE instead of explicitly hardcoding the platforms currently implementing it).
  2. Remove CacheBestSize() from there, it's already done by wxWindowBasE::GetBestSize().
  3. Important: avoid platform-specific code in wxChoiceBase. The whole idea of having it is to avoid these ugly #ifdefs. Instead, the base class version should contain only the code really common to all platforms and wxMSW should still continue to override it and handle wxCB_SIMPLE by adding the extra margin there (as for wxGTK, I don't think it's special case is needed at all but I could be wrong).

Could you please update the patch like this? TIA!

comment:8 follow-up: Changed 2 years ago by mmarsan

#if !wxUSE_GENERIC_TEXTMEASURE may not be yet defined for the time wxChoiceBase gets compiled: But __WXMSW__ __WXGTK__ are.

comment:9 in reply to: ↑ 8 Changed 2 years ago by mmarsan

Replying to mmarsan:

#if !wxUSE_GENERIC_TEXTMEASURE may not be yet defined for the time wxChoiceBase gets compiled: But __WXMSW__ __WXGTK__ are.

Forget this comment. wx/private/textmeasure.h is included so wxUSE_GENERIC_TEXTMEASURE is defined.

Changed 2 years ago by mmarsan

comment:10 Changed 2 years ago by mmarsan

4th version. I hope it is the last :)

comment:11 Changed 2 years ago by vadz

  • Status changed from new to confirmed

Thanks, much better but I don't really understand why do we need to test for wxUSE_GENERIC_TEXTMEASURE, wxTextMeasure is supposed to work in this case too, so I've just removed it. Please let me know if I broke something.

I also don't see any need for the code dealing with wxFont explicitly, using the window font should be the default behaviour anyhow and if it isn't, then it's a problem in wxTextMeasure itself and should be fixed there.

comment:12 Changed 2 years ago by vadz

For the reference, here is the code I used for testing:

  • samples/minimal/minimal.cpp

    diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp
    index a78e462..89b4682 100644
    a b bool MyApp::OnInit() 
    172172    CreateStatusBar(2); 
    173173    SetStatusText("Welcome to wxWidgets!"); 
    174174#endif // wxUSE_STATUSBAR 
     175 
     176    wxString choices[] = { "first", "very very very very long", "last" }; 
     177    wxSizer* sizer = new wxBoxSizer(wxVERTICAL); 
     178 
     179    wxChoice* c1 = new wxChoice(this, wxID_ANY); 
     180    sizer->Add(c1); 
     181 
     182    wxChoice* c2 = new wxChoice(this, wxID_ANY); 
     183    c2->Append(WXSIZEOF(choices), choices); 
     184    sizer->Add(c2); 
     185 
     186    wxChoice* c3 = new wxChoice(this, wxID_ANY); 
     187    c3->SetFont(wxNORMAL_FONT->Scaled(2)); 
     188    c3->Append(WXSIZEOF(choices), choices); 
     189    sizer->Add(c3); 
     190 
     191    SetSizerAndFit(sizer); 
    175192} 
    176193 
    177194 

comment:13 Changed 2 years ago by vadz

Ugh, testing under MSW shows that the best size is broken, the last choice above is now truncated... Looking into it...

comment:15 Changed 2 years ago by VZ

(In [72844]) Allow constructing wxGTK wxTextMeasure with NULL font.

The font is explicitly documented as being possibly NULL in the base class and
wxMSW handles NULL font just fine, so also handle it in the GTK version.

See #14706.

comment:16 Changed 2 years ago by VZ

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

(In [72848]) Refactor and simplify wxChoice::DoGetBestSize().

Use wxTextMeasure instead of duplicating its code and also reuse the code
between different ports.

Closes #14706.

Note: See TracTickets for help on using tickets.