Closed Bug 1208470 Opened 4 years ago Closed 4 years ago

HistoryPanel: Use Android resource system to pick layout

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: sebastian, Assigned: shubhamkjain, Mentored)

References

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 file, 7 obsolete files)

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
Version: Trunk → unspecified
Attached image Accent set to @color/action_orange (obsolete) —
Oops was also working on this bug. uploaded the pick in the wrong place
Oops, was also working on this bug. uploaded the pick in the wrong place
Attachment #8666392 - Attachment is obsolete: true
I fixed this bug in my local repository, i shall merge it tomorrow. followed the instructions you gave :)
Assignee: nobody → shubhamkjain
Attachment #8666632 - Attachment is obsolete: true
Attachment #8666645 - Flags: review?(s.kaspari)
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.
Attachment #8666645 - Flags: review?(s.kaspari) → review-
Attached patch patch_v3.diff (obsolete) — Splinter Review
refined the patch. Great experience! learned how mercurial actually worked :)
Attachment #8666819 - Flags: review?(s.kaspari)
Attachment #8666645 - Attachment is obsolete: true
Attachment #8666819 - Attachment is obsolete: true
Attachment #8666819 - Flags: review?(s.kaspari)
(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?
Flags: needinfo?(shubhamkjain)
Attached patch patch_v3.diff (obsolete) — Splinter Review
Yes, forgot to remove spaces.
Flags: needinfo?(shubhamkjain)
Attachment #8666851 - Flags: review?(s.kaspari)
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
Attachment #8666851 - Flags: review?(s.kaspari) → review+
Thank you. I'll gradually improve my logging skills :)
Attached patch patch_v4.diff (obsolete) — Splinter Review
Attachment #8666851 - Attachment is obsolete: true
Attachment #8667222 - Flags: review+
the patch didn't work for Android 2.3 opt and debug, right?
(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.
Attached patch patch-v5.diff (obsolete) — Splinter Review
Attachment #8667222 - Attachment is obsolete: true
Comment on attachment 8667846 [details] [diff] [review]
patch-v5.diff

I've added range_list to ids.xml
Attachment #8667846 - Flags: review?(s.kaspari)
Comment on attachment 8667846 [details] [diff] [review]
patch-v5.diff

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

Now everything passed. Great! :)
Attachment #8667846 - Flags: review?(s.kaspari) → review+
Cool, thank you for your support!
i really enjoyed learning the workflow, I'm hoping of learning a lot more in the near future!
Keywords: checkin-needed
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
Flags: needinfo?(shubhamkjain)
Keywords: checkin-needed
i don't really understand why the patch failed to apply. Can you please elaborate the reason?
Flags: needinfo?(shubhamkjain) → needinfo?(s.kaspari)
(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.
Flags: needinfo?(s.kaspari)
i still don't understand why i can't add 'range_list' to ids.xml.
Flags: needinfo?(s.kaspari)
(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. :)
Flags: needinfo?(s.kaspari) → needinfo?(shubhamkjain)
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.. :(
Flags: needinfo?(shubhamkjain)
Attached patch patch-v7.diffSplinter Review
I hope this patch works :)
Attachment #8667846 - Attachment is obsolete: true
Attachment #8672452 - Flags: review?(s.kaspari)
Comment on attachment 8672452 [details] [diff] [review]
patch-v7.diff

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

This one applies cleanly. :)
Attachment #8672452 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/mozilla-central/rev/b310a8f86728
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.