[Bug] SMS Style Search in Coverflow Views (1 Viewer)

arion_p

Retired Team Member
  • Premium Supporter
  • February 7, 2007
    3,373
    1,626
    Athens
    Home Country
    Greece Greece
    I still disagree. Not resetting SMS key state on selection change is in itself a bug as well.

    ResetSearchString is named appropriately and it does what its name suggests. Anytime the search string is cleared the previous and current key should be cleared too. And that should happen every time the selection changes EXCEPT when the change is a result of SMS search. That last part is the issue: this exception is not implemented.

    In fact SMS search does not work properly in any list control (list/thumb/filmstrip). It only allows typing the first letter whereas by looking at the code you can see that the intent was to be able to type more than just the first letter. Also linked to this is another bug(?): when searching the info label does not show the title of the selected/found entry but rather it shows the search string (which is already displayed above the list).

    A proper fix would be to make sure ResetSearchString is not called when selection changes as a result of a call to SearchItem. Also I have suggested in the past to rework all list controls inherit from a base list control where common functionality (such as SMS searching) can be implemented only once.
     

    SilentException

    Retired Team Member
  • Premium Supporter
  • October 27, 2008
    2,617
    1,130
    Rijeka, Croatia
    Home Country
    Croatia Croatia
    arion_p, sure, the fix for original issue can be done in two ways. - Either leave method as is and implement 3 or 4 exceptions (cannot remember exact number now) or remove clearing of previous and current keys from method and implement 1 exception (like in the patch).

    The two or more letters bug is obvious and I noticed it long long time ago but it's not related to this. If we are to implement the rework of controls with some base functionality, it could be fixed then.

    Another "bug" that you're talking about (#selecteditem) is not a bug I would say, but intended behavior someone wanted and implemented from start. Personally, I don't think that it was a bad decision.
     

    arion_p

    Retired Team Member
  • Premium Supporter
  • February 7, 2007
    3,373
    1,626
    Athens
    Home Country
    Greece Greece
    arion_p, sure, the fix for original issue can be done in two ways. - Either leave method as is and implement 3 or 4 exceptions (cannot remember exact number now) or remove clearing of previous and current keys from method and implement 1 exception (like in the patch).
    It is just 1 exception: when calling SearchItem().
    But the real root cause of the issue is that ResetSearchString is called in OnDefaultAction which is called for any action not handled (such as pressing a remote numpad key e.g. REMOTE_4). I am not sure in which additional cases ResetSearchString should be called (besides the ones already handled in OnAction) but it definitely should NOT be called in OnDefaultAction.


    The two or more letters bug is obvious and I noticed it long long time ago but it's not related to this. If we are to implement the rework of controls with some base functionality, it could be fixed then.

    In fact it is related: it is caused by ResetSearchString being called as a result of SearchItem(). Adding that one exception noted above solves both issues (in addition to OnDefaultAction fix which affects other list controls too).

    Another "bug" that you're talking about (#selecteditem) is not a bug I would say, but intended behavior someone wanted and implemented from start. Personally, I don't think that it was a bad decision.

    Doing further testing, it seems the issue exists in My Pictures but not My Videos nor My Music. Did not test everywhere so there may be other instances as well. In this case (My Pictures) I fail to see how displaying the search string in 2 places and hiding the currently selected item name is helpful in any way :confused:.
    Edit: See screen shot for an example:
    16-32-26.jpg

    In any case here is a patch to solve the original issue without the aforementioned side-effects. It is not a solution I particularly like but I think it is better than the originally proposed one.
     

    Attachments

    • Coverflow SMS Search fix.patch
      1.2 KB

    SilentException

    Retired Team Member
  • Premium Supporter
  • October 27, 2008
    2,617
    1,130
    Rijeka, Croatia
    Home Country
    Croatia Croatia
    Don't we have inconsistency with the patch now? CoverFlow will work with more than one letter, other facade controls not?

    About the #selecteditem, I like it because it shows current search string. I think it was done this way so that skins don't have to be modified. Actually, the correct way to do it would be to show search string only until SMS timer runs out. That way you would still see search string without modifying all skin files that use facade but it would be hidden after 1 second of not typing or after any user action.
     

    arion_p

    Retired Team Member
  • Premium Supporter
  • February 7, 2007
    3,373
    1,626
    Athens
    Home Country
    Greece Greece
    If we are to apply this patch, analogous fix would be applied to all other list controls - I just wanted to provide a more appropriate solution, it is by no means a full solution.

    About the #selecteditem: please do compare coverflow in MyVideoss and MyPictures:

    • in MyVideos, on the top left you see the search string, and under coverflow (bottom center) you see the title of the currently selected (found) item
    • in MyPictures, you see the search string in bothe the tope left of the screen and under coverflow. The selected item title is not displayed anywhere so you do not know which item has been found, just its thumb (which it may not have)
    Clearly one of the above behaviours is wrong and IMO it is the 2nd one.

    PS: IIRC the issue is not specific to coverflow view
     

    SilentException

    Retired Team Member
  • Premium Supporter
  • October 27, 2008
    2,617
    1,130
    Rijeka, Croatia
    Home Country
    Croatia Croatia
    I cannot check right now but what you're saying looks like skin thing. I'm just guessing because I'm leaving home right now and cannot check: in MyPictures #selecteditem might be used for title below CoverFlow whereas in MyVideos, internal CoverFlow label is used (that's why it shows item title). Top left label contains #selecteditem in both skins.
     

    Wo0zy

    Retired Team Member
  • Premium Supporter
  • April 30, 2008
    394
    134
    Home Country
    United Kingdom United Kingdom
    arion_p, sure, the fix for original issue can be done in two ways. - Either leave method as is and implement 3 or 4 exceptions (cannot remember exact number now) or remove clearing of previous and current keys from method and implement 1 exception (like in the patch).
    It is just 1 exception: when calling SearchItem().
    But the real root cause of the issue is that ResetSearchString is called in OnDefaultAction which is called for any action not handled (such as pressing a remote numpad key e.g. REMOTE_4). I am not sure in which additional cases ResetSearchString should be called (besides the ones already handled in OnAction) but it definitely should NOT be called in OnDefaultAction.


    The two or more letters bug is obvious and I noticed it long long time ago but it's not related to this. If we are to implement the rework of controls with some base functionality, it could be fixed then.

    In fact it is related: it is caused by ResetSearchString being called as a result of SearchItem(). Adding that one exception noted above solves both issues (in addition to OnDefaultAction fix which affects other list controls too).

    Another "bug" that you're talking about (#selecteditem) is not a bug I would say, but intended behavior someone wanted and implemented from start. Personally, I don't think that it was a bad decision.

    Doing further testing, it seems the issue exists in My Pictures but not My Videos nor My Music. Did not test everywhere so there may be other instances as well. In this case (My Pictures) I fail to see how displaying the search string in 2 places and hiding the currently selected item name is helpful in any way :confused:.
    Edit: See screen shot for an example:
    View attachment 87081

    In any case here is a patch to solve the original issue without the aforementioned side-effects. It is not a solution I particularly like but I think it is better than the originally proposed one.

    Hi arion_p,

    I don't want to get in the middle of a discussion between SE and yourself over this as there's absolutely nothing I can offer at the level you guys are talking :).

    Just wanted to let you know that I tested your patch and it isn't working the way I would expect. From what I can see the SMS search (first letter only) works once but if you then attempt a second search without first pressing a "non-SMS" key then nothing happens.

    For example, I search for movies beginning with "B" (press "2" twice) and I'm taken to the first movie beginning with "B". If I then immediately search for a movie beginning with say "N", no matter how many times I press "6" nothing happens. If I use a cursor key in between the first and second search it works fine.

    I applied your patch to a clean latest SVN. Is that correct or did I miss something?

    Also, should the patch have enabled SMS search on more than the first letter in myvideos using coverflow or did I misunderstand that bit?

    This was only a quick test before leaving work tonight so I'm sorry if I missed something in my rush.

    Thanks to you and SE for looking into this.

    All the best,

    Mick.
     

    arion_p

    Retired Team Member
  • Premium Supporter
  • February 7, 2007
    3,373
    1,626
    Athens
    Home Country
    Greece Greece
    Just wanted to let you know that I tested your patch and it isn't working the way I would expect. From what I can see the SMS search (first letter only) works once but if you then attempt a second search without first pressing a "non-SMS" key then nothing happens.

    For example, I search for movies beginning with "B" (press "2" twice) and I'm taken to the first movie beginning with "B". If I then immediately search for a movie beginning with say "N", no matter how many times I press "6" nothing happens. If I use a cursor key in between the first and second search it works fine.

    SMS search was intended to work just like SMS typing, i.e when you press 2 twice then 6 twice you are searching for an item containing "BN" (e.g. aBNormal). If none exists nothing happens. You can see the current search string in braces (like this: {bn}) usually on the top left over the facade. You can use backspace or # to erase the last character in the search string.

    To start a new search just move to a new item and start typing again.

    Btw, currently the patch only affects coverflow.
     

    Wo0zy

    Retired Team Member
  • Premium Supporter
  • April 30, 2008
    394
    134
    Home Country
    United Kingdom United Kingdom
    Just wanted to let you know that I tested your patch and it isn't working the way I would expect. From what I can see the SMS search (first letter only) works once but if you then attempt a second search without first pressing a "non-SMS" key then nothing happens.

    For example, I search for movies beginning with "B" (press "2" twice) and I'm taken to the first movie beginning with "B". If I then immediately search for a movie beginning with say "N", no matter how many times I press "6" nothing happens. If I use a cursor key in between the first and second search it works fine.

    SMS search was intended to work just like SMS typing, i.e when you press 2 twice then 6 twice you are searching for an item containing "BN" (e.g. aBNormal). If none exists nothing happens. You can see the current search string in braces (like this: {bn}) usually on the top left over the facade. You can use backspace or # to erase the last character in the search string.

    To start a new search just move to a new item and start typing again.

    Btw, currently the patch only affects coverflow.

    Thanks arion_p. That explains it.

    I tested with StreamedMP and the search string (in {braces}) isn't showing up. Just tried it with Default (where they do) and it makes perfect sense.

    I am aware that this is only in coverflow for the moment. Hopefully the function will make it to the other views soon!

    One more thing (sorry). Don't know if you've noticed but when using the SMS search, the title you "jump to" doesn't show its fanart in the background. If you move to it manually the fanart is there.

    :D again.

    Mick.
     

    Wo0zy

    Retired Team Member
  • Premium Supporter
  • April 30, 2008
    394
    134
    Home Country
    United Kingdom United Kingdom
    Hi guys,

    Did this get any further attention internally?

    Thanks,

    Mick
     

    Users who are viewing this thread

    Top Bottom