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. 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() (even though it sounds so nice!)
 
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
>