1208470 - HistoryPanel: Use Android resource system to pick layout

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Follow-up from bug 1142171.

In HistoryPanel we are picking the layout (normal vs. tablet landscape) in code:

>        if (HardwareUtils.isTablet() && GeckoScreenOrientation.getInstance().getAndroidOrientation() == Configuration.ORIENTATION_LANDSCAPE) {
>            return inflater.inflate(R.layout.home_history_split_pane_panel, container, false);
>        } else {
>            return inflater.inflate(R.layout.home_history_panel, container, false);
>        }
https://dxr.mozilla.org/mozilla-central/rev/e5effeb8e57ceddf679f7784faab0c2cebb1e1e6/mobile/android/base/home/HistoryPanel.java#123-127

Instead we could utilize Android's resource qualifiers:
* Use the same file name for home_history_split_pane_panel, home_history_panel
* Place the main layout in res/layouts and the tablet landscape version in something like res/layout-sw600dp-land.

http://developer.android.com/guide/topics/resources/providing-resources.html#QualifierRules

Comment 1

[Shubham Jain]

Attachment: Accent set to @color/action_orange

Comment 2

[Shubham Jain]

Oops was also working on this bug. uploaded the pick in the wrong place

Comment 3

[Shubham Jain]

Oops, was also working on this bug. uploaded the pick in the wrong place

Comment 4

[Shubham Jain]

I fixed this bug in my local repository, i shall merge it tomorrow. followed the instructions you gave :)

Comment 5

[Richard Newman [:rnewman]]

Bug 1024717, Bug 1022668…

Comment 6

[Shubham Jain]

Attachment: My first attempt in making a patch. :)

Comment 7

[Shubham Jain]

Attachment: Match made using hg this time. It includes the delete operation as well!

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8666645 [details] [diff] [review]
Match made using hg this time. It includes the delete operation as well!

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

Thank you for the patch! :)

Have you tested it sucessfully on different devices/emulators?

There are some files in the patch that look like they have been added accidentally.

::: mobile/android/base/home/HistoryPanel.java
@@ +123,2 @@
>              return inflater.inflate(R.layout.home_history_panel, container, false);
> +

NIT: Please remove this empty line.

::: mobile/android/base/resources/layout/home_history_split_pane_panel.xml
@@ -1,1 @@
> -<?xml version="1.0" encoding="utf-8"?>

Instead of removing this file and adding it again as home_history_panel.xml, use the "hg move" command to move the file. Otherwise mercurial will think that this file has been removed and a different file has been added.

::: mobile/android/gradle/gradle.iml
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8"?>

This file should not be part of the patch.

::: mobile/android/gradle/local.properties
@@ +1,1 @@
> +## This file is automatically generated by Android Studio.

This also should not be committed.

Comment 9

[Shubham Jain]

Attachment: patch_v3.diff

refined the patch. Great experience! learned how mercurial actually worked :)

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Shubham Jain from comment #9)
> Created attachment 8666819 [details] [diff] [review]
> patch_v3.diff
> 
> refined the patch. Great experience! learned how mercurial actually worked :)

You just marked the patch as obsolete and removed the review flag. Was this intended?

Comment 11

[Shubham Jain]

Attachment: patch_v3.diff

Yes, forgot to remove spaces.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8666851 [details] [diff] [review]
patch_v3.diff

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

Great patch. Thank you!

The commit message is super long. Use the first line to describe the change, this can be short (like a bug title, actually often they are the same). Then after a blank line you can (optionally) add additional text describing your intention, why you made decisions, etc. The git book has a very good chapter on commit messages: http://git-scm.com/book/ch5-2.html

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=509cea30211d

Comment 14

[Shubham Jain]

Thank you. I'll gradually improve my logging skills :)

Comment 15

[Shubham Jain]

Attachment: patch_v4.diff

Comment 16

[Shubham Jain]

the patch didn't work for Android 2.3 opt and debug, right?

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Shubham Jain from comment #16)
> the patch didn't work for Android 2.3 opt and debug, right?

Oh yeah. For Android 2.3 we are stripping a bunch of resources that are not needed on these devices. So that means that the layout-sw600dp-land folder will be stripped. The ID "range_list" is only used in the layout-sw600dp-land version of the layout, so it will be missing and cause a build error when accessed it HistoryPanel.

We might be able to work around this by adding the id to values/ids.xml.

Comment 18

[Shubham Jain]

Attachment: patch-v5.diff

Comment 19

[Shubham Jain]

Comment on attachment 8667846 [details] [diff] [review]
patch-v5.diff

I've added range_list to ids.xml

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Thank you! I pushed the new patch to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d08dc3d7c007

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8667846 [details] [diff] [review]
patch-v5.diff

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

Now everything passed. Great! :)

Comment 22

[Shubham Jain]

Cool, thank you for your support!
i really enjoyed learning the workflow, I'm hoping of learning a lot more in the near future!

Comment 23

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

renamed 1208470 -> patch-v5.diff
applying patch-v5.diff
patching file mobile/android/base/resources/values/ids.xml
Hunk #1 FAILED at 3
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/resources/values/ids.xml.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh patch-v5.diff

Comment 24

[Shubham Jain]

i don't really understand why the patch failed to apply. Can you please elaborate the reason?

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Shubham Jain from comment #24)
> i don't really understand why the patch failed to apply. Can you please
> elaborate the reason?

In short: Your patch is incompatible with the current state of the repository. Either this file has changed while you worked on it or you have been working on an older version of the file. You will need to edit your patch so that it applies cleanly.

There are multiple ways to do it. For example:

1) If you have a clean and up-to-date repository (See hg pull, hg update): Import the patch yourself. It will fail too. Perform the necessary changes. And commit again (--amend probably). Then create a new patch. If you are working with patch queues then you can also do this with mq: See hg qimport, hg qrefresh.

2) Pull the latest changes (hg pull) and rebase (hg rebase). This will fail when applying your patch. Fix the problems and continue the rebase. Then create a new patch.

3) There are probably a bunch of more ways for "conflict resolution" in mercurial.

Have a look at the Mercurial Book and the "Mercurial for Mozillians" guide:

http://hgbook.red-bean.com/read/
http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/index.html

Conflicts and fixing them can be confusing at the beginning. Take care that you do not lose any of your (other?) work. The patch that is attached here is safe. :)

As there are not many changes: In the worst case "re-do" the changes on top of an up-to-date repository.

Comment 26

[Shubham Jain]

i still don't understand why i can't add 'range_list' to ids.xml.

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Shubham Jain from comment #26)
> i still don't understand why i can't add 'range_list' to ids.xml.

You can - but the patch you attached can't be applied to the current ids.xml file. Since it looks like the file has not been edited by someone else, maybe you created the patch on top of an already locally changed ids.xml? Or isn't this the patch where you manually edited the commit message inside the patch file? Maybe this corrupted the patch?

However all you need to do is to pull the latest changes and re-create your patch on top of that. If you need some help then just ask in #mobile. :)

Comment 28

[Shubham Jain]

I had a problem with my Macbook's display 3 days a ago. Apple service providers will take about 4 days to get it replaced. I'm trying to setup a firefox repo on another laptop, but it's taking a lot of time for the downloading part.. :(

Comment 29

[Shubham Jain]

Attachment: patch-v7.diff

I hope this patch works :)

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8672452 [details] [diff] [review]
patch-v7.diff

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

This one applies cleanly. :)

Comment 31

[Pulsebot]

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

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b310a8f86728