949902 - Command to go up a level in bookmark view is mislabeled

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

In about:home, I tap Bookmarks and see folders labelled Desktop Bookmarks, Bookmarks Toolbar, etc.  I tap the folder labelled Desktop Bookmarks and see... a folder labelled Desktop Bookmarks.  I tap that folder again, and it takes me back to the previous list.  The icon is always the folder icon.

I thought this would say "Back" or "Up" or dots or something.

This is with a custom m-i build, based on 55e604639526.

Comment 1

[:Margaret Leibovic]

ibarlow, can you take a look at this and let us know if this was built to spec?

I agree with nalexander that it's kinda odd, so we should probably do something to improve this.

Comment 2

[Ian Barlow (:ibarlow)]

Yeah we can do better here. The second level actually does use a different icon, it's a folder with an arrow in it, but I agree we can make this more visually clear. I've made a new arrow asset here for "up a level" interactions. http://cl.ly/2I2n0x1q2T29

We should also, as Nick suggested, add some text to help. I would suggest trying "Up to [parent folder name]"

Comment 3

[Shashank VRSN Sabniveesu]

Hi! I traced down the occurrence of the string "Desktop Bookmarks" to this file -- mobile/android//base/home/BookmarksListAdapter.java

Can someone point me what I should be looking at?

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to shashank16392 from comment #3)
> Hi! I traced down the occurrence of the string "Desktop Bookmarks" to this
> file -- mobile/android//base/home/BookmarksListAdapter.java
> 
> Can someone point me what I should be looking at?

Hi, I assume you've already built Fennec locally? If not, see instructions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

You'll have to replace the images in mobile/android/base/resources/drawable-*/bookmark_folder_opened.png with the new arrows that ibarlow posted in the bug. Then, you'll have to change BookmarksListAdapter to show an 'Up to FOLDER_NAME' string instead of just the folder name. See:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BookmarksListAdapter.java#226

Comment 5

[Shashank VRSN Sabniveesu]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> (In reply to shashank16392 from comment #3)
> > Hi! I traced down the occurrence of the string "Desktop Bookmarks" to this
> > file -- mobile/android//base/home/BookmarksListAdapter.java
> > 
> > Can someone point me what I should be looking at?
> 
> Hi, I assume you've already built Fennec locally? If not, see instructions
> here:
> https://wiki.mozilla.org/Mobile/Fennec/Android
Yes, I've built it

> You'll have to replace the images in
> mobile/android/base/resources/drawable-*/bookmark_folder_opened.png with the
> new arrows that ibarlow posted in the bug.
I've replaced the arrows and they're showing up as expected

> Then, you'll have to change BookmarksListAdapter to show an 'Up to FOLDER_NAME' string instead of just
> the folder name. See:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> BookmarksListAdapter.java#226
I'm stuck here. 

If I add the string "Up to " at the #231, it shows up for all user-defined sub-folders, always;
            (section starting with) #238, it shows up for all occurences of the FOLDER_NAME always;
                                and #249, it never showed up
NOTE: By 'always', I mean 'irrespective of whether the folder is open or closed'. As I understand, they have to be differentiated as of this bug report.

ADDITIONAL COMMENTS:
In the case of icons, there's a provision to know whether a folder is "open" or "closed". See:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable/bookmark_folder.xml#9
I see no such possibility for the strings.

Also, Mr lucasr, may I know your nick on IRC?

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to shashank16392 from comment #5)
> (In reply to Lucas Rocha (:lucasr) from comment #4)
> > (In reply to shashank16392 from comment #3)
> > > Hi! I traced down the occurrence of the string "Desktop Bookmarks" to this
> > > file -- mobile/android//base/home/BookmarksListAdapter.java
> > > 
> > > Can someone point me what I should be looking at?
> > 
> > Hi, I assume you've already built Fennec locally? If not, see instructions
> > here:
> > https://wiki.mozilla.org/Mobile/Fennec/Android
> Yes, I've built it
> 
> > You'll have to replace the images in
> > mobile/android/base/resources/drawable-*/bookmark_folder_opened.png with the
> > new arrows that ibarlow posted in the bug.
> I've replaced the arrows and they're showing up as expected
> 
> > Then, you'll have to change BookmarksListAdapter to show an 'Up to FOLDER_NAME' string instead of just
> > the folder name. See:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> > BookmarksListAdapter.java#226
> I'm stuck here. 
> 
> If I add the string "Up to " at the #231, it shows up for all user-defined
> sub-folders, always;
>             (section starting with) #238, it shows up for all occurences of
> the FOLDER_NAME always;
>                                 and #249, it never showed up
> NOTE: By 'always', I mean 'irrespective of whether the folder is open or
> closed'. As I understand, they have to be differentiated as of this bug
> report.
> 
> ADDITIONAL COMMENTS:
> In the case of icons, there's a provision to know whether a folder is "open"
> or "closed". See:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable/bookmark_folder.xml#9
> I see no such possibility for the strings.

You're right, this should be added to the Folder view's text directly instead of the string itself. So, I think you'll have to append 'Up to' here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BookmarksListAdapter.java#291

> Also, Mr lucasr, may I know your nick on IRC?

It's lucasr :-)

Comment 7

[Shashank VRSN Sabniveesu]

Attachment: BUG 949902 - Patch detailing 3 new icons and a string.

I'm attaching the patch detailing modified icons and a string.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8424219 [details] [diff] [review]
BUG 949902 - Patch detailing 3 new icons and a string.

Review of attachment 8424219 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, still needs some tweaks.

::: mobile/android/base/home/BookmarksListAdapter.java
@@ +287,5 @@
>              row.updateFromCursor(cursor);
>          } else {
>              final BookmarkFolderView row = (BookmarkFolderView) view;
>              if (cursor == null) {
> +                row.setText("Up to " + mParentStack.peek().title);

This string has to be translatable. You'll have to use a string with params here. You can probably reuse this existing string for consistency:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#361

If you're wonder how you can use such type of string, your code should be analogous to:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/PanelBackItemView.java#44

Comment 9

[Shashank VRSN Sabniveesu]

Attachment: BUG949902.patch - Patch detailing 3 new icons and a string (Translatable).

Modified previous patch to contain translatable string

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8425831 [details] [diff] [review]
BUG949902.patch - Patch detailing 3 new icons and a string (Translatable).

Review of attachment 8425831 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking pretty good. Not sure about the image updates though (missed this in my previous review, sorry). Here's what propose:
- Update the bookmark_folder.xml drawable to use folder_up instead of bookmark_folder_opened
- Remove bookmark_folder_opened (double-check that it's being used anywhere else)

Please send a screenshot of the UI with your patch applied so that ibarlow can review it.

::: mobile/android/base/home/BookmarksListAdapter.java
@@ +267,5 @@
>  
>      @Override
>      public void bindView(View view, Context context, int position) {
>          final int viewType = getItemViewType(position);
> +        Resources res = context.getResources();

Declare this variable inside the 'if' block where it's being used.

Comment 11

[Shashank VRSN Sabniveesu]

(In reply to Lucas Rocha (:lucasr) from comment #10)
> - Update the bookmark_folder.xml drawable to use folder_up instead of
> bookmark_folder_opened
I never earlier saw that the icons 'folder_up.png' (mdpi, hdpi & xhdpi) existed; Now I do. Were these pushed recently?
I was replacing my 'bookmark_folder_opened.png' with the icon set uploaded here!
> - Remove bookmark_folder_opened (double-check that it's being used anywhere
> else)
So, in essence, you mean we would totally delete the 'bookmark_folder_opened.png' (mdpi, hdpi & xhdpi)?
> 
> ::: mobile/android/base/home/BookmarksListAdapter.java
> @@ +267,5 @@
> >  
> >      @Override
> >      public void bindView(View view, Context context, int position) {
> >          final int viewType = getItemViewType(position);
> > +        Resources res = context.getResources();
> 
> Declare this variable inside the 'if' block where it's being used.
I would do this; Can you let me the intention?

I felt that such a declaration at the very beginning of a method, would allow for any further (if newly introduced later) entities to make use of.

Comment 12

[Shashank VRSN Sabniveesu]

Attachment: BUG949902 fix: Animated Screenshot

The 'animated gif' shows the scenario after fixing BUG 949902, in action. The relevant patch would be attached after this

Comment 13

[Shashank VRSN Sabniveesu]

Attachment: BUG949902.patch - Patch for 3 icons updated, 3 icons deleted and a translatable string updated

Updated references to 3 icons, removed 3 icons as they are no more used(verified) and a translatable string updated

Comment 14

[Ian Barlow (:ibarlow)]

(In reply to shashank16392 from comment #12)
> Created attachment 8427850 [details]
> BUG949902 fix: Animated Screenshot
> 
> The 'animated gif' shows the scenario after fixing BUG 949902, in action.
> The relevant patch would be attached after this

Thanks for the animated gif! This looks nice, but we should change the labels to read "Up to [parent folder name]", instead of "Up to [name of folder I am currently in]" which isn't quite right.

Comment 15

[Shashank VRSN Sabniveesu]

Attachment: BUG949902 fix: Animated Screenshot -- Updated

[Updated] Animated Screenshot; Patch being attached after this

Comment 16

[Shashank VRSN Sabniveesu]

Attachment: [Updated] BUG949902.patch - Patch for 3 icons updated, 3 icons deleted and a translatable string updated

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 8428188 [details] [diff] [review]
[Updated] BUG949902.patch - Patch for 3 icons updated, 3 icons deleted and a translatable string updated

Review of attachment 8428188 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Just needs these final tweaks/fixes and we're probably go to go.

::: mobile/android/base/home/BookmarksListAdapter.java
@@ +288,5 @@
>          } else {
>              final BookmarkFolderView row = (BookmarkFolderView) view;
>              if (cursor == null) {
> +                Resources res = context.getResources();
> +                row.setText(res.getString(R.string.home_move_up_to_filter, (mParentStack.size()>2? mParentStack.get(1).title: res.getString(R.string.bookmarks_title)) ));

This should be > 1, no?

Nit: add spaces around '?' and ':'. Define title in a separate variable to make this code easier to read. Something like:

String title = (mParentStack.size() > 1 ? mParentStack.get(1).title : res.getString(R.string.bookmarks_title));
row.setText(res.getString(R.string.home_move_up_to_filter, title));

Comment 18

[Shashank VRSN Sabniveesu]

(In reply to Lucas Rocha (:lucasr) from comment #17)
>
> Review of attachment 8428188 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Just needs these final tweaks/fixes and we're probably go to go.
> 
> ::: mobile/android/base/home/BookmarksListAdapter.java
> @@ +288,5 @@
> >          } else {
> >              final BookmarkFolderView row = (BookmarkFolderView) view;
> >              if (cursor == null) {
> > +                Resources res = context.getResources();
> > +                row.setText(res.getString(R.string.home_move_up_to_filter, (mParentStack.size()>2? mParentStack.get(1).title: res.getString(R.string.bookmarks_title)) ));
> 
> This should be > 1, no?
No. The condition won't "fail" as expected since stack size becomes just equal to 2 when we enter the 'Desktop Bookmarks' folder. This shows an empty string "Up to" -- with no further text! Please see the next image attachment (debugging output)

Explanation:
------------

         While we're at            mParentStackSize()                   Comments
        ----------------          --------------------                 ----------
  Bookmarks (The 'Bookmarks tray') expected as 1         None (No "Up to.."; We're at the topmost)

     Desktop Bookmarks             "seen" to be 2        The "Up to.." needs a suffix that's not on 
                                                       stack; Hence only the conditional statement

       <Any further>                      >2             The "Up to.." gets a suffix that's on
                                                       stack
 
> Nit: add spaces around '?' and ':'. 
Done.

> Define title in a separate variable to
> make this code easier to read.
Done.
> Something like:
> String title = (mParentStack.size() > 1 ? mParentStack.get(1).title :
> res.getString(R.string.bookmarks_title));
> row.setText(res.getString(R.string.home_move_up_to_filter, title));

Comment 19

[Shashank VRSN Sabniveesu]

Attachment: Debugging output - After entering 'Desktop Bookmarks'

Debugging output - After entering 'Desktop Bookmarks' which is only next to the level of the top 'Bookmarks tray'

Comment 20

[Shashank VRSN Sabniveesu]

Attachment: BUG 949902: Corrected labels and icons to 'go up a level' in Bookmarks view; deleted 'bookmark_folder_opened.png' from drawable-(m|h|xh)dpi folders as its lone usage is replaced by this patch; code re

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 8429640 [details] [diff] [review]
BUG 949902: Corrected labels and icons to 'go up a level' in Bookmarks view; deleted 'bookmark_folder_opened.png' from drawable-(m|h|xh)dpi folders as its lone usage is replaced by this patch; code re

Review of attachment 8429640 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there! Sorry for the extra review loops, I missed some important details of this feature in my previous review. Patch should be good to go with the suggested changes below.

::: mobile/android/base/home/BookmarksListAdapter.java
@@ +288,5 @@
>          } else {
>              final BookmarkFolderView row = (BookmarkFolderView) view;
>              if (cursor == null) {
> +                Resources res = context.getResources();
> +                String title = (mParentStack.size()>2 ? mParentStack.get(1).title : res.getString(R.string.bookmarks_title));

Now I see what you mean. The empty title for size() > 1 happens because we don't currently set a title for the 'root' item in the parent stack. It seems we should change that now. Just change this constructor here:

http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BookmarksPanel.java#191

To be something like:

public BookmarksLoader(Context context) {
    final String title = getResources().getString(R.string.bookmarks_title);
    mFolderInfo = new FolderInfo(Bookmarks.FIXED_ROOT_ID, title);
    mRefreshType = RefreshType.CHILD;
}

If you do this, then you can just do:

row.setText(res.getString(R.string.home_move_up_to_filter, mParentStack.get(1).title));

As this is guaranteed to only be called when the parent stack's size is greater than 1 anyway.

Comment 22

[Ian Barlow (:ibarlow)]

Comment on attachment 8428186 [details]
BUG949902 fix: Animated Screenshot -- Updated

Looks good

Comment 23

[Shashank VRSN Sabniveesu]

(In reply to Lucas Rocha (:lucasr) from comment #21)
> 
> Almost there! Sorry for the extra review loops, I missed some important
> details of this feature in my previous review.
You are helpful all this way, patiently giving out sample code which duly guided me. I thank you more in return

> I see what you mean. The empty title for size() > 1 happens because we
> don't currently set a title for the 'root' item in the parent stack.
I'm excited that someone is keenly reading my analyses.

> It seems we should change that now. Just change the constructor
> If you do this, then you can just do:
> row.setText(res.getString(R.string.home_move_up_to_filter,
> mParentStack.get(1).title));
> 
> As this is guaranteed to only be called when the parent stack's size is
> greater than 1 anyway.
This eliminates the need for that crazy 'conditional' and makes the flow look better. Thanks for the suggestion.

I'm attaching a new patch. Please have a look at my comments coming along with it

Comment 24

[Shashank VRSN Sabniveesu]

Attachment: BUG949902.patch - Patch for 3 icons updated, 3 icons deleted, a translatable string updated and constructor for mParentStack of Bookmarks updated

Mr lucasr,

I have a few comments on this patch:

1) The recent patch resulted in -
-            this(context, new FolderInfo(Bookmarks.FIXED_ROOT_ID), RefreshType.CHILD);
+            super(context);
+            Resources res = context.getResources();
+            final String title = res.getString(R.string.bookmarks_title);
+            mFolderInfo = new FolderInfo(Bookmarks.FIXED_ROOT_ID, title);
+            mRefreshType = RefreshType.CHILD;

Rather we can have

-            this(context, new FolderInfo(Bookmarks.FIXED_ROOT_ID), RefreshType.CHILD);
+            this(context, new FolderInfo(Bookmarks.FIXED_ROOT_ID, context.getResources().getString(R.string.bookmarks_title), RefreshType.CHILD);

I only see, from the above suggestion, the 'readability of the code' is affected for one line but for that the existing 'handing over the call to\
 next constructor' just stays as it is. Either way is fine with me

2) Also,
     <!-- state open-->
      <item gecko:state_open="true">
-       android:drawable="@drawable/bookmark_folder_opened"/>
+       android:drawable="@drawable/folder_up"/>

     <!-- state close -->
-     <item android:drawable="@drawable/bookmark_folder_closed"/>

Now that this patch deletes the 'bookmark_folder_closed images' themselves from the repository entirely, and the actual images with names 'bookmark_folder_closed' look as 'any simple folder', wouldn't it be better if we rename them simply as 'folder' or 'folder_closed'? There is nothing specific with these images to 'Bookmarks'

Comment 25

[Lucas Rocha (:lucasr)]

(In reply to Shashank VRSN Sabniveesu from comment #24)
> Created attachment 8430403 [details] [diff] [review]
> BUG949902.patch - Patch for 3 icons updated, 3 icons deleted, a translatable
> string updated and constructor for mParentStack of Bookmarks updated
> 
> Mr lucasr,
> 
> I have a few comments on this patch:
> 
> 1) The recent patch resulted in -
> -            this(context, new FolderInfo(Bookmarks.FIXED_ROOT_ID),
> RefreshType.CHILD);
> +            super(context);
> +            Resources res = context.getResources();
> +            final String title = res.getString(R.string.bookmarks_title);
> +            mFolderInfo = new FolderInfo(Bookmarks.FIXED_ROOT_ID, title);
> +            mRefreshType = RefreshType.CHILD;
> 
> Rather we can have
> 
> -            this(context, new FolderInfo(Bookmarks.FIXED_ROOT_ID),
> RefreshType.CHILD);
> +            this(context, new FolderInfo(Bookmarks.FIXED_ROOT_ID,
> context.getResources().getString(R.string.bookmarks_title),
> RefreshType.CHILD);
> 
> I only see, from the above suggestion, the 'readability of the code' is
> affected for one line but for that the existing 'handing over the call to\
>  next constructor' just stays as it is. Either way is fine with me

I don't feel strongly about either approach. Up to you :-)

> 2) Also,
>      <!-- state open-->
>       <item gecko:state_open="true">
> -       android:drawable="@drawable/bookmark_folder_opened"/>
> +       android:drawable="@drawable/folder_up"/>
> 
>      <!-- state close -->
> -     <item android:drawable="@drawable/bookmark_folder_closed"/>
> 
> Now that this patch deletes the 'bookmark_folder_closed images' themselves
> from the repository entirely, and the actual images with names
> 'bookmark_folder_closed' look as 'any simple folder', wouldn't it be better
> if we rename them simply as 'folder' or 'folder_closed'? There is nothing
> specific with these images to 'Bookmarks'

You mean renaming the bookmark_folder_opened image to simply folder? Yes, we can do this. Please file a separate bug.

Comment 26

[Lucas Rocha (:lucasr)]

Comment on attachment 8430403 [details] [diff] [review]
BUG949902.patch - Patch for 3 icons updated, 3 icons deleted, a translatable string updated and constructor for mParentStack of Bookmarks updated

Review of attachment 8430403 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Please upload a new patch with the nits fixed.

::: mobile/android/base/home/BookmarksListAdapter.java
@@ +287,5 @@
>              row.updateFromCursor(cursor);
>          } else {
>              final BookmarkFolderView row = (BookmarkFolderView) view;
>              if (cursor == null) {
> +                Resources res = context.getResources();

nit: make 'res' final

::: mobile/android/base/home/BookmarksPanel.java
@@ +189,5 @@
>          private final RefreshType mRefreshType;
>  
>          public BookmarksLoader(Context context) {
> +            super(context);
> +            Resources res = context.getResources();

nit: make 'res' final

@@ +192,5 @@
> +            super(context);
> +            Resources res = context.getResources();
> +            final String title = res.getString(R.string.bookmarks_title);
> +            mFolderInfo = new FolderInfo(Bookmarks.FIXED_ROOT_ID, title);
> +            mRefreshType = RefreshType.CHILD;

As I said, feel free to just redirect the call to the other constructor in one line. Up to you.

Comment 27

[Shashank VRSN Sabniveesu]

Attachment: BUG 949902: Corrected labels and icons to 'go up a level' in Bookmarks view; deleted orphaned images; r=lucasr

Sorry for the delayed submission. I haven't seen that there's a second comment asking for an updated patch and hence have been waiting for check-in!

Looking back at the comment #17, I take back my proposal-1 which makes code more weird than that 'String title' statement.

Comment 28

[Lucas Rocha (:lucasr)]

Comment on attachment 8433587 [details] [diff] [review]
BUG 949902: Corrected labels and icons to 'go up a level' in Bookmarks view; deleted orphaned images; r=lucasr

Review of attachment 8433587 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks. We're in the last week of the Fx32 cycle so let's wait until next week to land this.

Comment 29

[Lucas Rocha (:lucasr)]

Just to be clear: the reason for waiting is to avoid any last minute regressions in Fx32.

Comment 30

[Lucas Rocha (:lucasr)]

After discussion on IRC, we decided to land this now as it's an easy back out if it causes any regression. Adding checkin-needed tag.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Do you have a Try link handy by chance? :)

Comment 32

[Shashank VRSN Sabniveesu]

Yeah! Uploaded it yesterday to test my new privileges!

https://tbpl.mozilla.org/?tree=Try&rev=ce462ad6cfb2

Is this what I've to report here? Not sure if I am supposed to do so

Comment 33

[Ryan VanderMeulen [:RyanVM]]

That's fine, just please include it when requesting checkin in the future :). Also, keep in mind that your patch only touches Android code, so building and testing every platform under the sun needlessly consumes a *LOT* of machine time and contributes to backlogs felt by all other devs. Please try to be more conscientious about that.
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Also, your Try push is showing robocop failures on every Android platform.
https://tbpl.mozilla.org/php/getParsedLog.php?id=41095132&tree=Try

You'll need to fix those before this can land.

Comment 34

[Shashank VRSN Sabniveesu]

It is only that due to the changed 'display texts', the previously written test cases are failing. Also many others are already failing which are not related to this patch but I see that we're only trying to reduce such failures as much as possible.

I could see that the call http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testBookmarkFolders.java#43
expects the old text [before this patch] which is straight away the name of the folder - 'Bookmarks Toolbar' to be present. Now this patch modifies it into 'Desktop Bookmarks' moreover with an additional string 'Up to' which is not being provided by the 'StringHelper'

I replaced 
        clickOnBookmarkFolder(StringHelper.TOOLBAR_FOLDER_LABEL);
with          
        clickOnBookmarkFolder(StringHelper.DESKTOP_FOLDER_LABEL);

As can be seen, this does fail; Now I'm groping to feel the correct placement of "Up to" and still unsure if I'm modifying the right call. Can someone guide me with these mochitest-robocop tests?

The functionality was demonstrated in the Attachment #842816 [https://bugzilla.mozilla.org/attachment.cgi?id=8428186]. Is it allowable to let the 'fxing of tests' be a different bug?

Comment 35

[Ryan VanderMeulen [:RyanVM]]

I know nothing about this code. Lucas is going to have to help.

Comment 36

[Lucas Rocha (:lucasr)]

(In reply to Shashank VRSN Sabniveesu from comment #34)
> It is only that due to the changed 'display texts', the previously written
> test cases are failing. Also many others are already failing which are not
> related to this patch but I see that we're only trying to reduce such
> failures as much as possible.
> 
> I could see that the call
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/
> testBookmarkFolders.java#43
> expects the old text [before this patch] which is straight away the name of
> the folder - 'Bookmarks Toolbar' to be present. Now this patch modifies it
> into 'Desktop Bookmarks' moreover with an additional string 'Up to' which is
> not being provided by the 'StringHelper'
> 
> I replaced 
>         clickOnBookmarkFolder(StringHelper.TOOLBAR_FOLDER_LABEL);
> with          
>         clickOnBookmarkFolder(StringHelper.DESKTOP_FOLDER_LABEL);
> 
> As can be seen, this does fail; Now I'm groping to feel the correct
> placement of "Up to" and still unsure if I'm modifying the right call. Can
> someone guide me with these mochitest-robocop tests?

Here's what you have to do:
- Add a BOOKMARKS_UP_TO constant to StringHelper with the value "Up to %s"
- Add a BOOKMARKS_ROOT_LABEL with the value "Bookmarks"
- Update testBookmarkFolders to use String.format(StringHelper.BOOKMARKS_UP_TO, ###) where ### is the appropriate folder label for each case. For example, for going up to the root folder, ### should be BOOKMARKS_ROOT_LABEL.

Send me the updated patch (including these test changes) and I can submit to our Try server to see if they are working fine.

Comment 37

[Shashank VRSN Sabniveesu]

Attachment: BUG 949902 - Corrected "Up to" strings in Bookmarks View; Deleted unused icons; Tests updated r=lucasr

Tests updated as directed

Comment 38

[Lucas Rocha (:lucasr)]

Pushed a fixed version of the patch to Try:
https://tbpl.mozilla.org/?tree=Try&rev=ed667ba0cab0

Comment 39

[Lucas Rocha (:lucasr)]

Comment on attachment 8440000 [details] [diff] [review]
BUG 949902 - Corrected "Up to" strings in Bookmarks View; Deleted unused icons; Tests updated r=lucasr

Review of attachment 8440000 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with the suggested fixes.

::: mobile/android/base/tests/testBookmarkFolders.java
@@ +39,5 @@
>  
>          clickOnBookmarkFolder(StringHelper.TOOLBAR_FOLDER_LABEL);
>  
>          // Go up in the bookmark folder hierarchy
> +        clickOnBookmarkFolder(StringHelper.BOOKMARKS_UP_TO, DESKTOP_FOLDER_LABEL);

This should actually be:

clickOnBookmarkFolder(String.format(StringHelper.BOOKMARKS_UP_TO,
                                    StringHelper.DESKTOP_FOLDER_LABEL));

@@ +44,3 @@
>          mAsserter.ok(waitForText(StringHelper.BOOKMARKS_MENU_FOLDER_LABEL), "Going up in the folder hierarchy", "We are back in the Desktop Bookmarks folder");
>  
> +        clickOnBookmarkFolder(StringHelper.BOOKMARKS_UP_TO, BOOKMARKS_ROOT_FOLDER);

This should be:

clickOnBookmarkFolder(String.format(StringHelper.BOOKMARKS_UP_TO,
                                    StringHelper.BOOKMARKS_ROOT_LABEL));

Comment 40

[Shashank VRSN Sabniveesu]

Attachment: BUG 949902 - Added "Up to" strings in Bookmarks View; Deleted unused icons; Tests updated r=lucasr

Comment 41

[Mark Capella [:capella]]

looked good to me ... I built and checked it on my S3 and N7.

I'm still unable to run robocop tests locally, as I have the same issue you ran into recently: ( bug 1026830 ), so I pushed to try:

Results indicate you have one last test to correct ...
https://tbpl.mozilla.org/?tree=Try&rev=3f66fc35d911

Comment 42

[Lucas Rocha (:lucasr)]

Comment on attachment 8441502 [details] [diff] [review]
BUG 949902 - Added "Up to" strings in Bookmarks View; Deleted unused icons; Tests updated r=lucasr

Review of attachment 8441502 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine but it seems the test is still failing. We need to figure what's going on here.

Comment 43

[Shashank VRSN Sabniveesu]

Attachment: BUG 949902 - Corrected "Up to" strings in Bookmarks View; Deleted unused icons; Tests updated r=lucasr

All tests passed :)

https://tbpl.mozilla.org/?tree=Try&rev=6a2a34631965

Comment 44

[Lucas Rocha (:lucasr)]

Comment on attachment 8442555 [details] [diff] [review]
BUG 949902 - Corrected "Up to" strings in Bookmarks View; Deleted unused icons; Tests updated r=lucasr

Review of attachment 8442555 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome :-)

Comment 45

[Lucas Rocha (:lucasr)]

Please add the checkin-needed tag.

Comment 46

[Shashank VRSN Sabniveesu]

(In reply to Lucas Rocha (:lucasr) from comment #44)
> Comment on attachment 8442555 [details] [diff] [review]
> BUG 949902 - Corrected "Up to" strings in Bookmarks View; Deleted unused
> icons; Tests updated r=lucasr
> 
> Review of attachment 8442555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome :-)

Thank you :)

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5b7a06134d30

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b7a06134d30