Generic MaskFormatter that handles AllowNull #119

errael · 2022-02-21T21:51:48Z

This has the basics. This mask formatter used in SSDateField and it's Factory.

Added Format enum, rather than DateFormat enum, probably controversial. Not sure how I feel about it, but in terms of allowing an app to specify it's own default formats this might be handy starting place. Can discuss in #18 or this PR.

Wanted to set initial state of SSDateField, after Nav's AddRecord button is pushed, to either null or current date depending on AllowNull. I put the logic in SSDateField.cleanField() but it never gets called (from anywhere). Any idea how this can be done?

At the very top there's SSFormatteTextField.VISIBLE_ERROR_STATE. When true, the field goes redish when it hold an invalid value. When false, it has the previous behavior and the field goes red when you try to leave the field and it's invalid. I had difficulty getting the new behavior to match the old behavior, any idea?

swingset/src/main/java/com/nqadmin/swingset/formatting/Format.java

bpangburn

Minor comments here and there. Still need to hammer on it a bit. Previous edition worked pretty well.

swingset/src/main/java/com/nqadmin/swingset/formatting/Format.java

bpangburn · 2022-02-23T16:22:04Z

swingset/src/main/java/com/nqadmin/swingset/formatting/SSDateField.java

@@ -50,12 +50,16 @@

 	/**
 	 * constant representing the dd/mm/yyyy date format
+	 * @deprecated


Where we deprecate things and there is a replacement method/constructor, I'm a fan of * @deprecated use {@link #replacementMethod() } JavaDoc. Several examples in SSDBComboBox.

Left the unadorned @deprecated as a reminder which I forgot.

Strangely @deprecated use {@link Format.DATE_DDMMYYYY} fails javadoc build. Javadoc enum handling problem? Maybe you can come up with something.

Doing it like @deprecated use {@link Format}'s DATE_DDMMYYYY

bpangburn · 2022-02-23T16:25:47Z

swingset/src/main/java/com/nqadmin/swingset/formatting/SSDateFormatterFactory.java

 	 */
-	public static final int YYYYMMDD = 2;
+	// Don't like the way default is buried in get(Format) case label.


Does the need for a default format for each general type swing the enum approach one way or the other? I guess you're handing this with Format.DATE.

Right, left over comment before adding Format.DATE to signify default.

The single enum approach would allow something like SSFormattedTextField.get(Format.DATE) to return a text field with default formatter for the given type. Another reason is to make it easier to specify the default per app, rather than spreading it around in the different places FormatterFactory.

I think you mentioned somewhere that there could be an enum that just has the types like DATE/MONEY/... and then separate enums for each sub-type. That would work as well, the default sub-type would be specified as part of the type enum; that does feel cleaner and gives more compile time checking. It also gives a single spot for specifying the default sub-type for a given type.

After all the other issues are addressed, I'll put together a change just for the enums. Please speak up with any naming and/or other conventions in this area.

In com.nqadmin.swingset.enums we use mixed case class enum name and all caps items, which is what you were doing with FORMAT enum. I guess we could consider a .formatting.enus package, but that might be overkill. Should we try to use the delimiter in the item names? YYYYMMDD_DELIM_HYPEN? Probably not. Other formatters might have multiple delimiters/separators (e.g., currency might have "." for cents and "," for thousands). Just trying to list some things that might cause enum ambiguity or confusion if we're going to handle order and delimiters, etc. in a single enum item.

One possibility (blue sky) would be to only have FieldType enum. Then something like

SSFormattedTextField DateField.get() returns something with reasonable defaults

SSFormattedTextField DateField.builder().mask("xxx").delimiter("").validator(doVal).build() if you don't want the defaults

Then you don't need any enums. But this might be to open ended especially considering that it has to work with the verifier; I guess the stuff like DATE_YYYYDDMM sets up a variety of things so it all works together, and is a convenience. But in general, each subType may have it's own set of options and that complicates things again.

It might be a good idea to consider/try a few more Type implementations. I'm getting confused as to what's needed.

swingset/src/main/java/com/nqadmin/swingset/formatting/SSDateField.java

bpangburn · 2022-02-23T22:48:36Z

swingset/src/main/java/com/nqadmin/swingset/formatting/SSDateField.java

+		// TODO Consider setting to null vs system date.
+		if (!getAllowNull()) {
+			setValue(new java.util.Date());
+		}


Do we want/need to call setValue(null) if getAllowNull()==true? Same for cleanField(). Maybe the constructor should just call cleanField() if we keep that method?

Does it matter what we do in the constructor? Nothing's bound yet, no idea what AllowNull is.

How about just don't do anything in the constructor and count on the rest of the system and/or app to take care of it?

Makes sense on the constructor since we don't know what getAllowNull() will give. What about cleanField() presuming we figure out how to call it when appropriate? Seems safest to set to null if allowed or you could get some unexpected dates propagating to the database. Would rather force the end user to fill explicitly.

Even if !Allowed, could set to null. Then it shows up red.

I've got a PR ready with the background color stuff working, empty constructor, cleanField called for new record. I'll push that and you can take a look, see how it feels.

swingset/src/main/java/com/nqadmin/swingset/formatting/SSFormattedTextField.java

bpangburn · 2022-02-23T23:05:45Z

swingset/src/main/java/com/nqadmin/swingset/formatting/SSMaskFormatterFactory.java

@@ -0,0 +1,418 @@
+/* *****************************************************************************


New method so will probably try to digest in Eclipse rather than in GitHub site.

FYI, SSMaskFormatterFactory had no changes from the first preview you saw.

SSDateField.cleanField uses AllowNull. Avoid potential NPE in updateSSComponent.

errael · 2022-02-24T04:54:06Z

This latest commit should take care of the basics. Then the background colors. By the time that's done, we should be able to decide on the format enums.

bpangburn · 2022-03-19T03:17:08Z

Curious if an email reply will result in a comment… We can pull this. Hopefully if we redo the Enums it won’t create merge issues, but seems like it should be fine.

…

On 2022-03-18 17:44, errael wrote: @errael commented on this pull request. ------------------------- In swingset/src/main/java/com/nqadmin/swingset/formatting/Format.java [1]: > @@ -0,0 +1,87 @@ +/* ***************************************************************************** I looked around, seems like the merge to 4.1.0 worked well. Does this enums thing have to be resolved before #119 [2] going into 4.1.0? Or can we resolve it as part of further 4.1.0 development? -- Reply to this email directly, view it on GitHub [1], or unsubscribe [3]. Triage notifications on the go with GitHub Mobile for iOS [4] or Android [5]. You are receiving this because your review was requested.Message ID: ***@***.***> Links: ------ [1] #119 (comment) [2] #119 [3] https://github.com/notifications/unsubscribe-auth/APVWSKACNGPYDAML6RV4EUDVAUBLDANCNFSM5O7VM2PQ [4] https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 [5] https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub

errael · 2022-03-19T15:09:37Z

We can pull this.

"pull"? Did you mean merge?

Hopefully if we redo the Enums it won’t create merge issues, but seems like it should be fine.

If we discuss/coordinate on the enums, I agree that we should be able to avoid merge hell.

bpangburn · 2022-03-19T16:48:03Z

@errael Yes. I guess that "Merge pull request" means you Request that I Pull your branch/changes into 4.1.0-SNAPSHOT, which Merges them into 4.1.0-SNAPSHOT? Merge sounds better :) and is a bit more intuitive.

I'll go ahead and merge this and then I may create a new branch based on the latest 4.1.0-SNAPSHOT to play with the colors/borders and then maybe start to look at another formatter (maybe SSN or Currency).

Generic MaskFormatter that handles AllowNull

1ce1cff

errael requested a review from bpangburn February 21, 2022 21:52

bpangburn reviewed Feb 23, 2022

View reviewed changes

swingset/src/main/java/com/nqadmin/swingset/formatting/Format.java Outdated Show resolved Hide resolved

bpangburn reviewed Feb 23, 2022

View reviewed changes

From review, clean up comments, use cleanField() when new record.

efcfb8b

SSDateField.cleanField uses AllowNull. Avoid potential NPE in updateSSComponent.

errael added 2 commits February 24, 2022 19:08

Fixup/cleanup dynamic visible error state for formatted text

188a18c

Review Fix

8a70816

bpangburn changed the base branch from 4.0.9-SNAPSHOTV2 to 4.1.0-SNAPSHOT March 17, 2022 14:19

bpangburn and others added 2 commits March 17, 2022 12:26

Merge branch '4.1.0-SNAPSHOT' into MaskFormatterNull

5d8547a

getLogger fix for SSMaskFormatterFactory

0d82ec5

Update Format.java

7636ac2

bpangburn merged commit 1e47bca into bpangburn:4.1.0-SNAPSHOT Mar 19, 2022

errael deleted the MaskFormatterNull branch March 19, 2022 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic MaskFormatter that handles AllowNull #119

Generic MaskFormatter that handles AllowNull #119

errael commented Feb 21, 2022 •

edited

Loading

bpangburn left a comment

bpangburn Feb 23, 2022

errael Feb 24, 2022

errael Feb 24, 2022

bpangburn Feb 23, 2022

errael Feb 24, 2022

bpangburn Feb 24, 2022

errael Feb 24, 2022

bpangburn Feb 23, 2022

errael Feb 24, 2022 •

edited

Loading

bpangburn Feb 24, 2022

errael Feb 24, 2022

bpangburn Feb 23, 2022

errael Feb 24, 2022

errael commented Feb 24, 2022

bpangburn commented Mar 19, 2022 via email

errael commented Mar 19, 2022

bpangburn commented Mar 19, 2022

		@@ -0,0 +1,418 @@
		/* *****************************************************************************

Generic MaskFormatter that handles AllowNull #119

Generic MaskFormatter that handles AllowNull #119

Conversation

errael commented Feb 21, 2022 • edited Loading

bpangburn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

errael Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

errael commented Feb 24, 2022

bpangburn commented Mar 19, 2022 via email

errael commented Mar 19, 2022

bpangburn commented Mar 19, 2022

errael commented Feb 21, 2022 •

edited

Loading

errael Feb 24, 2022 •

edited

Loading