Skip to content

- DateField emits onDateSelected event when date set #762

Closed
wants to merge 3 commits into from

2 participants

@mahomahomaho

as in subject. It's nice when I'm able to receive event when date is set in dateField.

@anthonyrisinger
Polyglot Pythonistas member

ok, this doesn't modify existing API -- should be fine -- but some notes/questions below:

  • what is GWT doing ATM? (this is likely an old version of theirs)
  • is this going to cause double events if the user clicks a date, or clicks the "today" button? (might be [un]expected behavior, depending on who you ask)
  • why should focus events fire this event?
  • whats up with the onTboxChanged and emitSelectedDate separation? tbox is more of an impl detail and doesn't need to be exposed ... why not merge onDateSelected and emitDateSelected? it seems to me that a new handler is not needed at all, and you simply want to be informed directly if the date changes?
@mahomahomaho
@anthonyrisinger
Polyglot Pythonistas member

I have no idea. As I'm not java/gwt user, I don't know even where to
search.

thats fine don't worry about it -- what you found is actually extjs-on-GWT (yuck), but no matter.

I just pushed fix to it (forgot to remember last_date).

ok good good.

My experience with textboxes and javascript says that "on lost
focus"/onblur is valuable source of information that text has changed
(there are scenarios where text is changed and input and changed events
are not fired).

fair enough, as long as you are recording the last date it should be fine.

Well, we could just add default-null parameter to emitDateSelected and
make it handler of input/changed events, but I think it's worth to keep
separation between "we selected date by click on calendar/today" and "we
wrote something into text, maybe valid maybe not". Maybe we will need to
add some kind of validation in latter?

well, this is javascript so you have to validate everything in the end anyway -- i don't think there is much gain by making the two separate (not that we shouldn't try to make it safe, code could def be added to abort the update/event if it's not a real date, see below).

my main holdup is that the interface is so much different than Calendar, and should be changed to match Calendar as much as possible:

  • rename listeners to changedDateListeners (mirror Calendar)
  • rename emitSelectedDate to onDate (mirror Calendar), update _last_date here, but DONT getText here ... signature should match onDate from calendar: event, yy, mm, dd
  • rename onTboxChanged to onFieldChanged, split the date and call onDate with the event (mirror Calendar). if date can't be split into yy/mm/dd, abort, and update tbox with _last_date (not awesome validation, but should be splittable at least)

does that make sense? for the most part it's just renames.

@mahomahomaho
@anthonyrisinger
Polyglot Pythonistas member

whoa whoa, slow down, we are straying here.

we don't need a new class -- it's superfluous. if you want to modify the addSelectedDateListener signature to accept a keyword argument, that is fine, but it should be descriptive, like useDatetime or asDatetime and needs to match the rest of the API -- hell if it we up to me nothing would even be CamelCase, but that just not how the API is, and straying from these expectations only makes things harder for everyone.

emitDateSelected needs to match the onDate handler from calendar. TBH, i don't think you even need the other calls to emitDateSelected -- the Today/onFocus/etc handlers are calling self.tbox.setText which will fire the onChanged textbox handler later on.

you are probably correct about aborting within the date changed handler, simply accepting the update is fine. it was just a last second idea i had but i didn't consider the handler would fire for each character.

so, action items:

  • drop the new class/etc ... this update needs to be the least possible changes to get the desired behavior.
  • consider storing the listeners as a tuple: (listener, useDatetime) ... or in separate attributes: selectedDateHandlers, selectedDatetimeHandlers
  • pass everything thru a single onDate method linked to change and focus events of the textbox -- there should be no need to call it manually because all other [successful] events eventually result in a self.tbox.setText. this name, onDate, matches other areas of the API (emit* is not used anywhere).
@mahomahomaho
@anthonyrisinger
Polyglot Pythonistas member

But ... if you want second (DateField) class to have same functions as
first class (Calendar), which in fact does the same ... isn't it logical
that that functionality should be moved to third, common place? I think
that copy&paste pattern is quite evil ...

So, are you sure you want situation that when you fix some bug in one
place (Calendar.addSelectedDateListener), you need to remember to fix it
in another (DateField.addSelectedDateListener) ?

well, i'm not really disagreeing with your reasoning, i just don't want to modify so much in one shot. TBH, i would prefer if Calendar was not changed at all, and you only added the bits you needed to DateField ... i'm trying to get the functionality you need with minimal change to the codebase. if you only need DateField for your purposes, don't even worry about Calendar. i know it sounds counter intuitive, but don't worry about what's "most correct", just do what's necessary to get what you need with minimal impact.

the thing is, this library is near frozen, no new functionality should really be added at all, as we are preparing for GWT 2.x+. each time we stray from the original GWT sources things get more difficult to maintain over time. once that process start (very soon) this part of the library will freeze and only receive bugfixes.

Ok, I agree with name change. I'm not 100% sure about not calling it
from onDateSelected handlers.

go ahead and try it out -- i think it should work fine, but if it it's causing problems i'll take a look, or we'll just go ahead and pull as-is.

@mahomahomaho
@mahomahomaho
@anthonyrisinger
Polyglot Pythonistas member

hrm ... ok **** it, we'll go ahead with this ... i'm not sure i'm 100% satisfied with the result, but that's ok.

just confirm the existing API is still intact (eg. existing code doesn't break) and it also does what you need, and i'll pull it.

thanks @mahomahomaho :-)

@mahomahomaho
@anthonyrisinger
Polyglot Pythonistas member

sure sure, just drop a line when your ready.

... and enjoy the hell out of your vacation :-)

@mahomahomaho

I just returned from vacation (well, it was month ago but I was buried with superurgent tasks), I reviewed the change and I'm pretty sure that API is intact.

So I think you can safely pull it into trunk.

@anthonyrisinger
Polyglot Pythonistas member

ok, i'll review this shortly and go from there (ie, prob just pull as-is)

@anthonyrisinger
Polyglot Pythonistas member

this is still relevant to you, yes?

could you rebase your work into a single, unified commit (with title/body message explaining the change, and ideally, how to use it)? i'm unable to merge it as-is for whatever reason, so you may have to resolve some conflicts and verify proper operation.

looking to handle this in the next few days, thanks for your patience, much appreciated.

@mahomahomaho
@mahomahomaho

I think I made mess only.

I'm going to re-create my fork from scratch, and make my changes again. I wonder if this pull request will disappear, or I will be able to "switch" to new repo in same pull request?

@anthonyrisinger
Polyglot Pythonistas member

nah, you can reuse this request, no problem -- this request is tied to the dfevents branch -- you just need to [force] push to this branch: git push -f origin dfevents.

@mahomahomaho

I think this pull request is broken too. It refers to commits which are no longer alive (repository deleted and another one, with new name created).

So I will close it, and reopen shadow of it, with new commits :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.