Where on github can I see comments? On Sun, Jan 30, 2011 at 2:49 AM, damien krotkine <dkrotkine@gmail.com> wrote:
Hi !
I've commented on github. Let's swithc to that for the following discussion indeed. Brian, you are more than welcome to participate :)
dams
On 29 January 2011 14:59, Flavio Poletti <polettix@gmail.com> wrote:
Hi all, I don't know if this mailing list is actually the right place to continue this discussion about this module implementation, let us know if you're bored! I implemented a generic approach and you can find it here: https://github.com/polettix/Dancer-Plugin-FlashMessage/tree/alternatives It is based on the configurability of three traits: * how messages are registered ("queue" option) * how additional parameters are handled ("arguments" option) * how messages are flushed away ("dequeue" option). There is a lot of new meat so I also added the full documentation with some examples. This implementation is kinda back-compatible, with two outstanding exceptions: * persistence has been dropped, even though its functionality has been absorbed by "dequeue" * the 'flash' method now only *sets* messages, and it never deletes them. IMHO, the case for deletion in the code is quite obscure, so I tried to move this functionality inside the new "flash_flush" registered keyword. It is anyway possible to revert to the old behaviour if it is considered better. For any other aspects, the current default values replicate the previous behaviour. If it makes sense to you we can add a plethora of tests. Thank you all for your patience, we'll probably better continue this discussion on GitHub if you agree. Flavio.
On Fri, Jan 28, 2011 at 7:05 PM, Flavio Poletti <polettix@gmail.com> wrote:
On Fri, Jan 28, 2011 at 6:52 PM, damien krotkine <dkrotkine@gmail.com> wrote:
On 28 January 2011 18:40, Flavio Poletti <polettix@gmail.com> wrote:
Hi,
the implementation is sound, even though I'd prefer to be able to set multiple messages per single channel. If I validate a form, for example, I'd like to flash a warning for each field I don't like, not just the last one.
Hm, my brain being what it is on friday night, could you elaborate a bit more ? can you give an example of such channels ?
I'm thinking you could use an array of string as flash.warnings, instead of a simple string, but I'm not sure I understand what you're saying
Sorry, you got the "channel" idea. I'd like to use flash like this:
flash warning => 'you made error X';
and then, possibly later, say:
flash warning => 'you made error Y';
In the current implementation, only the last gets to the template. I'm suggesting to always push the incoming $value into an array. Otherwise, with the current implementation that lets you delete stuff, I'd be forced to do it like this:
my $array = flash('warning') || []; push @$array, 'you made error X'; flash warning => $array;
which is a bit ugly...
On the other hand, deleting flash messages from the controller does seem to be a corner case, which I'd handle with an ad-hoc function or ignore completely.
I'll try to propose something on GitHub that lets the end user decide the preferred semantics through a configuration; I would personally like to avoid having leftovers in the flash when my templates are not bug-free and I definitely don't like the idea of remembering to call flash->flush()
I agree. After all, maybe it would make sense to have 3 behaviors : behaviour 1 : delete all the keys as soon as one is used (to be implemented ) behaviour 2 : delete only the key which is used ( current implementation ) behaviour 3 : never delete the keys ( currently persistence = 1 )
And maybe behaviour 1 should be the default, I don't know. As long as behaviour 2 is available, I'm happy (and brian too)
maybe the behaviour could be chosen with the persistence config keyword ? It could be renamed to flush_behaviour
flush_behaviour = key_level flush_behaviour = hash_level flush_behaviour = never
What do you think ? OK I'm terrible at finding good names, so any help welcome :)
I'd leave "_behaviour", add an 'always' (your previous implementation) and put something like:
flush = 'key' flush = 'all' flush = 'always' flush = 'never'
Anyway, as long as it's documented and it's reasonably connected to the behaviour everything is fine :-)
(even though it sounds so nice!)
aha, yes, I like flash->flush :) Let's add it for fun, even if it's not necessary :)
Cheers,
Flavio.
On Fri, Jan 28, 2011 at 6:33 PM, damien krotkine <dkrotkine@gmail.com> wrote:
I agree with Brian, for a slightly different reason :
Let's imagine this:
you have a Template1, that does this : <% flash.keys() %>, to be able to know that they are errors. If that's the case, it doesn't display the errors, but display a link, saying that they are erros. This link does a new request against the server, serving Template2
Template2 does use flash.error, and effectively consumes the message.
You don't want the flash hash structure to be emptied just after having displayed Template1, you want the messages to survive until Template2 (after all, you didn't consume them in Template1)
If I understand correctly, In the original implementation, the simple fact of getting the keys of the hash would delete all the messages, so the next request, Template2 won't be able to retrieve <% flash.error %>
In my new implementation, it would work.
Granted, this is a bit of a corner case, however, I don't see any drawback.
I could also add a flash->flush() method.
On 28 January 2011 18:09, Brian E. Lozier <brian@massassi.com> wrote: > I can't think of any real world use case where you'd only want to > show > certain messages. That said, it still seems better to only clear > the > messages you use, not all of them. > > On Fri, Jan 28, 2011 at 9:02 AM, Flavio Poletti <polettix@gmail.com> > wrote: >> Hi, >> >> of course we can try to adapt to it (it seems a bit difficult >> although >> probably not impossible), but I fail to see the point. >> >> IMHO, if you get the chance to show the messages, you should show >> them >> all: >> showing warnings in one page and errors in the following one would >> puzzle >> the end user, wouldn't it? >> >> Unless there is some use case that point in the opposite direction >> I'd >> stick >> to the "wipe them all when you get the chance to show them" >> semantics. >> >> Cheers, >> >> Flavio. >> >> On Fri, Jan 28, 2011 at 5:32 PM, Brian E. Lozier >> <brian@massassi.com> >> wrote: >>> >>> It looks like a pretty good solution. The only thing I can see >>> would >>> be that if you are in a template and check for flash.error but not >>> flash.warning, it looks like it will clear both flash.error and >>> flash.warning. It seems like it should only clear the one you're >>> checking for. >>> >>> >>> >>> On Fri, Jan 28, 2011 at 12:02 AM, Flavio Poletti >>> <polettix@gmail.com> >>> wrote: >>> > Hi, >>> > I thought a bit about it and I think that the current >>> > implementation >>> > is a >>> > bit too limited (this seems also what brian thinks in the bug >>> > report http://rt.cpan.org/Public/Bug/Display.html?id=65009 - at >>> > least >>> > from >>> > what I understand). >>> > The problem is that the flash message might have to be displayed >>> > in >>> > the >>> > next >>> > request, which happens when redirections kick in. In this case, >>> > the >>> > flash >>> > message should be set, kept in the session and removed only when >>> > it >>> > is >>> > actually displayed; this should happen automatically, i.e. >>> > without >>> > playing >>> > with the $persistent variable - which is global - or with >>> > further >>> > overhead >>> > on the controller side. >>> > Following brian's feedback, I tried to re-code the plugin with >>> > the >>> > following >>> > criteria: >>> > * the "flash" method only adds messages (and handles many of >>> > them >>> > for >>> > every >>> > category - you might have more than one error in a page). I see >>> > no >>> > point >>> > in >>> > removing messages while inside the controller; >>> > * the message removal logic is shifted in the template via an >>> > anonymous >>> > sub. >>> > If the sub is called then the flash cache is cleared, otherwise >>> > it >>> > remains >>> > there. This should guarantee that the messages are not removed >>> > until >>> > they >>> > get the chance to be displayed. >>> > This is the core of the recoding - not tested but should suffice >>> > to >>> > give >>> > you >>> > the idea: >>> > register flash => sub { >>> > my ($key, $value) = @_; >>> > my $flash = session($session_hash_key); >>> > if (! $flash) { # initialise the container for flash >>> > messages >>> > $flash = {}; >>> > session($session_hash_key, $flash); >>> > } >>> > push @{$flash->{$key}}, $value; >>> > return; >>> > }; >>> > before_template sub { >>> > my $obj = shift; >>> > my $flashes; >>> > $obj->{$token_name} = sub { >>> > if (! $flashes) { # first call, get messages and clear >>> > $flashes = session($session_hash_key) || {}; >>> > session($session_hash_key, {}); >>> > } >>> > return $flashes; >>> > }; >>> > return; >>> > }; >>> > >>> > IMHO there is no need for an "exists", in the template you can >>> > do >>> > like >>> > this: >>> > <% IF flash.errors %>...<% END %> >>> > The important thing to remember is that even checking for the >>> > presence >>> > of >>> > something inside "flash" clears the messages, which is >>> > consistent >>> > with >>> > the >>> > semantics that "they get cleared as soon as they have a chance >>> > to be >>> > displayed". The $flashes variable guarantees that they survive >>> > at >>> > least >>> > until the end of the request, i.e. you are able to call "flash" >>> > multiple >>> > times while handling a request and always get all messages. >>> > If you think that this makes sense, I can propose a pull request >>> > on >>> > GitHub >>> > or you can make the changes directly. >>> > Cheers, >>> > Flavio. >>> > >>> > >>> > On Tue, Jan 11, 2011 at 5:48 PM, damien krotkine >>> > <dkrotkine@gmail.com> >>> > wrote: >>> >> >>> >> I've re-implemented it to be more Rails-like, as sukria said. >>> >> >>> >> https://github.com/dams/Dancer-Plugin-FlashMessage >>> >> >>> >> and on CPAN, pending mirrors refresh. >>> >> >>> >> The funny part of the story ? the effective code is only 30 >>> >> lines >>> >> long. Talking about Perl and Dancer expressiveness... >>> >> >>> >> dams. >>> >> >>> >> On 11 January 2011 14:40, Alexis Sukrieh <sukria@sukria.net> >>> >> wrote: >>> >> > Hi list! >>> >> > >>> >> > Le 11/01/2011 14:29, damien krotkine a écrit : >>> >> >> >>> >> >> Hi, >>> >> >> >>> >> >> following previous thread, I've done a first implementation >>> >> >> of >>> >> >> Dancer::Plugin::FlashMessage : >>> >> >> >>> >> >> https://github.com/dams/Dancer-Plugin-FlashMessage >>> >> > >>> >> > Great! Thanks a lot for your time dams, the myth is still >>> >> > alived! >>> >> > (Dancer's >>> >> > community)++ >>> >> > >>> >> > >>> >> > >>> >> >> Some parts need to be improved, for instance : >>> >> >> >>> >> >> - it supports only one flash message >>> >> >> - the keywords are not short enough. >>> >> >> >>> >> >> So I think I'll change the implementation so that the >>> >> >> template >>> >> >> token >>> >> >> is >>> >> >> simply called 'flash', and it'll be a hash, like in Rails. >>> >> >> I'll >>> >> >> also >>> >> >> change the registered method so that it's just flash() >>> >> >> instead >>> >> >> of >>> >> >> get_flash() >>> >> > >>> >> > I agree. I'd like to behave just like Rails' flash feature. >>> >> > The >>> >> > idea >>> >> > is >>> >> > pretty straight forward: >>> >> > >>> >> > "flash" is an accessor to a particular session hash table >>> >> > whose >>> >> > values >>> >> > can >>> >> > only be accessed once. Nothing more complicated than that. >>> >> > >>> >> > So to conclude, IMO, flash should be a wrapper like the >>> >> > following: >>> >> > >>> >> > sub flash { >>> >> > my ($key, $value) = @_; >>> >> > my $flash = session('_flash'); >>> >> > >>> >> > # write >>> >> > if (@_ == 2) { >>> >> > $flash->{$key} = $value; >>> >> > session('_flash' => $flash); >>> >> > } >>> >> > >>> >> > # read (+ delete) >>> >> > else { >>> >> > my $value = $flash->{$key}; >>> >> > delete $flash->{$key} if defined $value; >>> >> > session('_flash' => $flash); >>> >> > } >>> >> > >>> >> > return $value; >>> >> > } >>> >> > >>> >> > This is it, I think. This allows for the following code in a >>> >> > Dancer >>> >> > app: >>> >> > >>> >> > >>> >> > get '/' => sub { >>> >> > flash welcome => "This is a welcome message, only >>> >> > shown >>> >> > once"; >>> >> > } >>> >> > >>> >> > Then, as soon as the key 'welcome' is accesed via >>> >> > flash('welcome'), >>> >> > the >>> >> > entry will be purged. >>> >> > >>> >> > This will be very helpful for authentication stuff in before >>> >> > filters, >>> >> > error >>> >> > messages, notifications, .... >>> >> > >>> >> > >>> >> > Kudos to dams! >>> >> > >>> >> > (BTW I haven't read the code yet) >>> >> > >>> >> > -- >>> >> > Alexis Sukrieh >>> >> > _______________________________________________ >>> >> > Dancer-users mailing list >>> >> > Dancer-users@perldancer.org >>> >> > http://www.backup-manager.org/cgi-bin/listinfo/dancer-users >>> >> > >>> >> _______________________________________________ >>> >> Dancer-users mailing list >>> >> Dancer-users@perldancer.org >>> >> http://www.backup-manager.org/cgi-bin/listinfo/dancer-users >>> > >>> > >>> > _______________________________________________ >>> > Dancer-users mailing list >>> > Dancer-users@perldancer.org >>> > http://www.backup-manager.org/cgi-bin/listinfo/dancer-users >>> > >>> > >>> _______________________________________________ >>> Dancer-users mailing list >>> Dancer-users@perldancer.org >>> http://www.backup-manager.org/cgi-bin/listinfo/dancer-users >> >> > _______________________________________________ > Dancer-users mailing list > Dancer-users@perldancer.org > http://www.backup-manager.org/cgi-bin/listinfo/dancer-users >