Bug 857942:[SMS] Select all button enabled in edit screen during new sms. r=borjasalguero #9064

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@borjasalguero borjasalguero commented on the diff Apr 9, 2013
apps/sms/js/activity_handler.js
return;
}
+ ThreadListUI.checkInputs();
@borjasalguero
borjasalguero Apr 9, 2013 Member

Why dont move only once before IF clause?

@rwaldron
rwaldron Apr 9, 2013 Contributor

These are different calls:

ThreadUI.checkInputs();
ThreadListUI.checkInputs();

@borjasalguero
borjasalguero Apr 9, 2013 Member

My fault! I read the same Object name.

@borjasalguero
borjasalguero Apr 10, 2013 Member

Hi again.
As a new message it's handled by 'onMessageReceived', I would fix there the bug. Here you have the scenarios:

  • Append a message if you are currently in the thread. We would need to add 'ThreadUI.checkInputs()' in line 80 of MessageManager.
  • When we are in 'edit-mode' in Thread-List, we have to add 'ThreadListUI.checkInputs() in line 115. Also if the new SMS belongs to one of the selected threads, we should keep the 'status' and restore it after rendering.
    We should should check if we are in 'edit-mode' (checking the hash it's enough) for calling `___.checkInputs()'. Please address the suggestion and I will test it again. Thanks!
@leob2g
leob2g commented Apr 9, 2013

@borjasalguero This issue fix is for two cases

1.If we are in particular Thread edit screen and got a new message then we need to enable "Select all".This case is handled by adding ThreadUI.checkList()
2.If we are in ThreadList edit screen and got a new message then we need to enable "Select all".For this case to handle added ThreadListUI.checkInputs()

Thanks,

@janjongboom
Member

I don't believe this should be in the activity handler. It's UI code that should be handled in the respective files (ThreadUI and ThreadListUI), they should be responsible for updating their UI at certain points.

@borjasalguero
Member

I think we could add ThreadUI.checkList() in ThreadUI.appendMessage, checking if we are in edit mode or not (with the hash for example) or, at least, in onMessageReceived method.

@fcampo
Contributor
fcampo commented Apr 9, 2013

There's another PR fixing the same issues #9056 which takes that approach, and includes some tests. Maybe there was a lack of sync between devs when fixing the bug?

Please assign the bug to yoursleves when you start to work on it so other devs won't work on the same :)

@rwaldron
Contributor
rwaldron commented Apr 9, 2013

@fcampo +1 to #9056

@borjasalguero
Member

Please add a test-case for this feature. @janjongboom has created other patch as well, could you sync in order to get only one PR to be reviewed? Thanks!

@leob2g
leob2g commented Apr 11, 2013

@borjasalguero

I will check this with your comments given

@janjongboom
Member

As far as I know #9056 now covers all the cases also covered in this issue.

@leob2g
leob2g commented Apr 12, 2013

@borjasalguero

Hi Borja,

I have done the changes with the comments given by you.I will test the patch once and upload it.

Thanks

@leob2g leob2g closed this May 2, 2013
@leob2g leob2g deleted the leob2g:Bug_857942_Select_all_Button_Not_Enabled_when_New_Sms_Received_Edit_Messages branch May 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment