Bugzilla – Bug 2246
Make showBriefly not exit until all text has been displayed
Last modified: 2008-09-15 14:39:24 UTC
If you do a showBriefly with text that doesn't fit on the display, the text scrolls, but it's very hard to work out how long to display it for so that all the text has time to be displayed. showBriefly should really regard its time parameter as a minimum time and should display for longer if the scroll still hasn't finished. I believe this should be the default behaviour, with a new option added to force the display time. Displaying arbitrary messages to the user is a very helpful feature of showBriefly. This feature would make things a lot easier.
I agree this would be useful, but I'm not sure that it should be the default behavior. Most uses of the showBriefly are for giving brief feedback based on button presses, where the user knows what's onthe display (they just chose to do something with it!) but we want to confirm that the action happened. We could have a "scrollBriefly()" animation that did the same thing as showBriefly but guaranteed one complete scroll. It might be useful to have an optional callback for when it's completed. Triode, what do you think? Also, Max, it would be great to understand the application for this scrolling feature so we can make sure you get all the capabilities you need.
Is it not the case that for nearly all uses so far the text wouldn't scroll anyway? In this case, changing the default behaviour wouldn't affect these things. In the cases where you're doing a showBriefly with more text than would fit on the display, why would you not want the user to be able to read it? That seems bad ui to me. I'm currently doing showbrieflys from random mix to display the album/artist that's just been added e.g. Adding to end of playlist / Random Album: The Dark Side of The Moon. I want the user to see the text, but I don't want it to hang around for any longer than it needs to. Basically, it should display for 2 seconds (as specified by me), unless scrolling is necessary, in which case it should scroll the whole text once, with the standard scroll pause at the end.
showBriefly is generally used to display for a very short time as a confirmation of action. Also, remember that after the showBriefly finishes, they'll see the scrolling anyways, right? Imagine the scenario where the user is browsing to a song title that is very long and they press PLAY. That means that they may see the Now Playing Very Long Song Title Here (featuring this artist and that artist,etc...) For a very long time. If they needed to see what the whole thing was, they would have done it before they pressed play. The problem gets much worse if they are in double-height mode. I can imagine the usefulness of a animation function that scrolls exactly once through or a minimum time, but it's not a replacement for showBriefly. Your example is kind of a hybrid, as you are showing something briefly that the user _hasn't_ seen before and won't see again.
Its definately possible to add this sort of animimation. At present showBriefly calls update() after the timeout to redisplay the old screen. We would need to change this (as an option or new function) to only occur if the timeout occurs or scrolling has completed. Max - if you want to demo yourself now you could implement this as a call to $client->update($display, 1) with your text in $display, then set a timer (to a value greater than the scrollPause) to check (!$client->scrollData || $client->scrollData->{paused}), else reset the time to check later. If true call $client->update() to display the normal screen. [if the background screen is doing periodic updates, you probably need to do $client->updateMode (1) straight after the first update.]
Created attachment 908 [details] patch to support this Max - patch as discussed on dev list. Hopefully you can see this!
Created attachment 913 [details] updated patch updated patch with scrollPause after scrolling before doing new update
Max - added to r4772. If you and Dean are happy please close bug. Please update RandomMix to use. Dean - if this & Max's patch look low risk, can we consider for 6.2.1?
I've checked in the trivial changes to Random Mix to use this new code - R4788. I'd certainly imagine this was low risk enough for 6.2.1. I'm happy for this bug to be closed, although it would be nice to see a version that didn't wrap the scroll at some distant point in the future. Thanks for your work, Adrian!
I'd prefer to keep the changes for 6.2.1 to a minimum, in part to get folks to 6.5 and also to reduce risk. I think having it pause at the end of the scroll and not wrap around would be a good thing. Adrian?
The version committed has the pause at the end of the scroll, but does use the return to start wrapping. We could change so the text scrolls off the left never to be seen again (like ticker mode, but it starts on the screen.) However in this case I don't think the pause at the end is appropriate as it will be pausing a blank screen! Let me see if I can come up with a patch for the above and you/Max can see which one you prefer [needs another option in the rendering code, but not impossible]
I think that it should pause for two seconds before starting the scroll, then when the last pixel in the line is visible, pause for two more seconds before reverting. Is this what it does?
It will currently do the wrap round scroll and uses the scrollPause preference: - Display text as normal, pause for scrollPause or 0.5 sec [whichever is larger] - Scroll round with wrapped text until the same text is displayed on the left edge - Pause for scrollPause or 0.5 sec [whichever is larger] - end animation and cause update Changing to something other than scrollPause means it can't reuse the normal scrolling code. I'd prefer to avoid that. What could be added is a new mode to render which means you don't get the wrap. Either: - no repeated text, scroll to all text exposed and then pause - pad with space so text scrolls off left, but with no pause but I'm not sure either is better than current?
Created attachment 944 [details] Scroll with pause but no wrap Max - try this, it removes the wrapping text from scrolling - it keeps the pause at the end. If you want to try the alternative (padding with space), remove "- $screensize" from the line: $cache->{endscroll} = $cache->{line2finish} - $screensize; NB I've not done the character display version, so try on a graphic player..
That patch is exactly what I wanted. It displays the message in as short an amount of time as it can get away with whilst still letting you read it. I'd really like to see this in the trunk. Thoughts, Dean? I haven't tried the space padded version.
Max, I've put this in trunk for Dean and others to see if they like it.. Also added hash based args for showBriefly [actually for the duration and flags]. You should use: $client->showBriefly( <display_hash>, { 'scroll' => 1 } ); or $client->showBriefly( <line1>, <line2>, { 'scroll' => 1 } ); [are you subscribed to the checkin list? If so I can stop posting updates here!] Close bug?
I'm perfectly happy with this fix. Thanks, Adrian. I'm afraid I don't yet subscribe to the checkins list. Either way, I still think it's useful for a bug to have all the relevant comments in it.