Opened 6 years ago

Last modified 4 months ago

#14535 confirmed enhancement

Masked edit v1

Reported by: mmarsan Owned by:
Priority: normal Milestone: 3.2.0
Component: GUI-generic Version: stable-latest
Keywords: masked edit control Cc: mmarsan@…
Blocked By: Blocking:
Patch: yes

Description

Here is the first version.

I attach a .patch file and also a .png file (for docs/doxygen/images).
I hope I have done it in the proper way.

Lacks:

  • No include/wx/univ/setup.h and setup.h_vms updates
  • No xrc (xh_*) files

Issues:

  • I can not make doxygen to build a good doc's structure (i.e. typedefs and predefined functions).
  • In wxGTK I can not get right lower case characters. In wxMSW there is no problem. Perhaps I have something misconfigurated in my machine.

Regards
Manolo

Attachments (4)

wxMaskedEdit_v1.pacth download (139.9 KB) - added by mmarsan 6 years ago.
maskededit.png download (1.3 KB) - added by mmarsan 6 years ago.
wxMaskedEdit_v1r1.patch download (142.8 KB) - added by mmarsan 6 years ago.
Revison 1: fix code and extension
wxMaskedEdit_v1r2.patch download (142.9 KB) - added by mmarsan 6 years ago.
Not use wxIsprint for 'C' and 'c' mask commands

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by mmarsan

Changed 6 years ago by mmarsan

comment:1 follow-up: Changed 6 years ago by vadz

  • Status changed from new to confirmed

I've finally had a first look at the patch and it looks globally good, thanks. Unfortunately the file in the Trac had a wrong extension so Trac doesn't highlight it automatically so I'll probably upload it to review.bakefile.org to view it there -- and maybe comment on it there too. Could you please create an account there (it just requires your email)?

For now I only see 2 small problems: first, MSVC gives this warning which seems to be worth looking into:

maskededit.cpp(1386) : warning C4146: unary minus operator applied to unsigned type, result still unsigned

Second, in spite of my expectations, the cursor doesn't jump to the next field automatically when you finish the entry of the current one. E.g. with the phone number example, if you type "123" you get "(123|)" in the control where I used pipe to indicate the caret position. I'd expect "(123)|".

Finally, it would be nice if the sample also showed the customization possibilities just to see what is available.

But globally, once again, this looks very good, the sample is very impressive!

P.S. I'd like to invite people to test this under OS X and GTK, while I'll try to do it myself too, it would be better if more people did it, especially under OS X.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 6 years ago by mmarsan

Sorry. I made a typo: the extension I sent was .'pacth' instead of '.patch'.

Why use bakefile.prg instead of wxWidgets?

maskededit.cpp(1386) : warning C4146: unary minus operator applied to unsigned type, result still unsigned

I presumed the compiler to cast a size_t to a long. I'll fix it.

The new caret position ought to be as you explain. And indeed this wrong behaviour happens only with right-aligned fields. Anyhow, if you continue typing, the chars are written in the right place. I'll fix it too.

Should I send a patch against the already submitted 'pacth' or should I send a full, new, right extension patch?

Finally, it would be nice if the sample also showed the customization possibilities just to see what is available.

I don't understand this. The example uses most possibilities, although it's not clear until you see the example's code. Could you explain a bit more?

But globally, once again, this looks very good, the sample is very impressive!

Thanks.

comment:3 in reply to: ↑ 2 Changed 6 years ago by vadz

Replying to mmarsan:

Why use bakefile.prg instead of wxWidgets?

The Review Board installation there allows to leave the comments directly associated with the patch lines. This is very useful for large(r) patches.

Should I send a patch against the already submitted 'pacth' or should I send a full, new, right extension patch?

I'd prefer if you could upload it to Review Board but if not, please post the entire new patch here.

Finally, it would be nice if the sample also showed the customization possibilities just to see what is available.

I don't understand this. The example uses most possibilities

I meant things like setting colours, bell on errors and such -- i.e. "customizations".

although it's not clear until you see the example's code.

This is a good point, perhaps it would make sense to have a short description of each control, e.g. "Date entry, notice that invalid day/month combinations are detected.".

Changed 6 years ago by mmarsan

Revison 1: fix code and extension

comment:4 Changed 6 years ago by mmarsan

Even I see the review.bakefile advantages, by now, IMHO, wxwidgets is the site to add patches.

I've fixed the code and added tool tips to sample.

I attach the revision 1 (full new) except the .png file, which is the same as before.

Regards,
Manolo

comment:5 Changed 6 years ago by mmarsan

Here is the revision 2 (full new) except the .png file, which is the same as before.

The main change is not using wxIsprint() for 'C' and 'c' mask commands.
It also fixes a few minor bugs, like avoid a "unused param" warning in the sample.

Changed 6 years ago by mmarsan

Not use wxIsprint for 'C' and 'c' mask commands

comment:6 Changed 5 years ago by vadz

  • Milestone changed from 2.9.5 to 3.2

Sorry, this won't make it into 2.9.5...

comment:7 Changed 5 years ago by vadz

For anybody interested in continuing to work on this, the latest changes from my local tree are also available at Github.

comment:8 Changed 4 months ago by vadz

Amazingly, this patch still seems to work fine for me, at least under wxGTK. I've rebased the PR on the latest master in order to check if it builds everywhere too and I'll try to (re?)review it soon.

Of course, if anybody else can have a look at this, it would be very welcome too.

comment:9 Changed 4 months ago by vadz

After actually testing it under Linux, it doesn't really work there (any more?), unfortunately. E.g. typing keys into various controls in the sample often just beeps instead of behaving correctly as it does under MSW, so something probably got broken in this code during wxGTK evolution and it's not obvious what exactly, just from looking at it (even if the code is relatively clear, there is a lot of it and it's difficult to debug it without spending more time than I have for it now).

I also realized that I couldn't really use it for my needs because it wasn't flexible enough. I think a better API would be decomposed in 2 parts: the low-level one allowing to fix some positions of the control to the given characters and a high-level one providing validation. E.g. I'd like to just append "%" to a text field, but allow the usual numeric input otherwise and, importantly, use an existing validator for it, but this doesn't seem to be possible right now because there is no such low-level API.

From the UI point of view, limitation to fixed width fonts is quite bothersome as it makes impossible to use such a control with normal text controls nearby without switching font for all of them, as otherwise it would be inconsistent and ugly (and, of course, using monospace globally is just a different kind of ugly). I don't know if this is something really fundamental or would be easy to change, in fact, I don't see why is this required at all.

Anyhow, this is definitely an impressive piece of work and it seems to work fine under MSW in my limited testing, so it could be already useful for some people, but we need to really make it work under Linux and Mac too and, ideally, improve the API, before merging it.

comment:10 Changed 4 months ago by mmarsan

  • Cc mmarsan@… added

wxGTK evolution is likely the culprit for those beeps. EVT_CHAR handling and IM handling is where I'd look at.

I'm not developing for Linux for some quite time. To debug this code first I need to update sources, even GTK+. I don't know when I'm going to do it. Sorry.

Validation is done at char level (i.e. OnChar()), field level, and control level. I don't see a clear way to do this with a validation API. I'm completely sure that validators are useless here, because not only validation at those levels doesn't fit well, but also because "paste" and "move cursor" actions are in a different flow.

Fixed width font is not required. I set it as default due to two reasons:
a) For a fixed number of chars (e.g. IP or serial key controls) I think it's clear for the user to see every "cell" of the same width.
b) Setting the size of the control (which sizers shouldn't change) is more accurate with fixed-width. I use an approximation (see maskededit.cpp:390) based on several tests I did. For fixed length cases, not fixed width fonts looked ugly.

Speaking of setting sizes, that code (.cpp:374) should be updated with
GetSizeFromTextSize()

Note: See TracTickets for help on using tickets.