Opened 7 years ago

Closed 7 years ago

#9126 closed enhancement (fixed)

wxImage: make type typesafe (enum wxBitmapType)

Reported by: troelsk Owned by:
Priority: low Milestone:
Component: GUI-all Version:
Keywords: Cc: troelsk, vadz
Blocked By: Blocking:
Patch: yes

Description

Patch:

  • enum wxBitmapType instead of long
  • Fixed one bad instance of FindHandler(-1), replaced with FindHandler(wxBITMAP_TYPE_ANY)
  • IsOk() instead of Ok()
  • wxT() instead of _T()
  • Added m_orig_type+GetOrigType(), to remember what LoadFile found

Notes:
Tested on MSW only, sorry. Bound to cause [trivial] compilation errors in the other ports. Unlikely to cause any problems in [correct] user code. Image sample compiles clean without modifications.

Attachments (2)

wximage-type.patch download (26.7 KB) - added by troelsk 7 years ago.
wximage-type-2.patch download (24.0 KB) - added by troelsk 7 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 years ago by troelsk

m_orig_data in wxImageRefData.
File Added: wximage-type.patch

comment:2 Changed 7 years ago by troelsk

  • wxImage::SaveFile(const wxString&) returned true unconditionally.
  • Changed GetOrigType() to LastType() and made both LoadFile() and SaveFile() set it

File Added: wximage-type.patch

Changed 7 years ago by troelsk

comment:3 Changed 7 years ago by troelsk

Bound to cause [trivial] compilation errors in the other ports

After another look: not necessarily any problems with the other ports.

Using 'wxBitmapType', not 'enum wxBitmapType'.

File Added: wximage-type.patch

comment:4 Changed 7 years ago by vadz

The 3 different patches in this patch (wxBitmapType, IsOk() and wxT()) really need to be separated... is it still possible for you to submit them separately? Also, I'd rather not apply the last one (wxT) at all, it's a completely gratitious change, both wxT and _T are used throughout wx code and I actually prefer _T in wxMSW.

Also, please add documentation of LastType() (if only to explain to _me_ what is it useful for), adding new public functions without documenting them is a really bad idea.

TIA!

Changed 7 years ago by troelsk

comment:5 Changed 7 years ago by troelsk

is it still possible for you to submit them separately?

Not really. Then I'd need to retype everything. I can offer only to take LastType out, which is painful, it is quite bad to be without it. wxImage is tossing away perfectly good information: If calling wxImage::LoadFile(filename,wxBITMAP_TYPE_ANY) all you are getting is a bool. No way of telling which handler hit jackpot. Hence LastType().

New patch:
wxBitmapType + IsOk() + wxT() but no LastType.
Please apply the original patch last to get LastType too.

LastType documentation:
"wxBitmapType LastType() const
Returns the type of bitmap file found by LoadFile, or specified with SaveFile"

File Added: wximage-type-2.patch

comment:6 Changed 7 years ago by troelsk

both wxT and _T are used throughout wx code

Ideally one of them should go, two is one too many.
(In the meantime, I saw that you've started not using either in cpp files - plain "" instead of wxT("") - this will take some getting used to)

comment:7 Changed 7 years ago by wojdyr

  • Component set to GUI-all
  • Type set to enhancement

"Unlikely to cause any problems in [correct] user code."
Changing long to enum in a few functions in API obviously will result in many build errors (like pen/brush styles) in user code.
I'm not saying it can't be done, but it would be good to test and fix it also on other ports.

comment:8 Changed 7 years ago by vadz

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

Untangled the other changes, updated the docs and committed as r53999.

Note: See TracTickets for help on using tickets.