HistoryPanel: Use Android resource system to pick layout

RESOLVED FIXED in Firefox 44

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sebastian, Assigned: Shubham Jain, Mentored)

Tracking

unspecified
Firefox 44
All
Android
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

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

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Version: Trunk → unspecified
(Assignee)

Comment 1

3 years ago
Created attachment 8666392 [details]
Accent set to  @color/action_orange
(Assignee)

Comment 2

3 years ago
Oops was also working on this bug. uploaded the pick in the wrong place
(Assignee)

Comment 3

3 years ago
Oops, was also working on this bug. uploaded the pick in the wrong place
(Reporter)

Updated

3 years ago
Attachment #8666392 - Attachment is obsolete: true
(Assignee)

Comment 4

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

Comment 6

3 years ago
Created attachment 8666632 [details] [diff] [review]
My first attempt in making a patch. :)
(Reporter)

Updated

3 years ago
Assignee: nobody → shubhamkjain
(Assignee)

Comment 7

3 years ago
Created attachment 8666645 [details] [diff] [review]
Match made using hg this time. It includes the delete operation as well!
(Assignee)

Updated

3 years ago
Attachment #8666632 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8666645 - Flags: review?(s.kaspari)
(Reporter)

Comment 8

3 years ago
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-
(Assignee)

Comment 9

3 years ago
Created attachment 8666819 [details] [diff] [review]
patch_v3.diff

refined the patch. Great experience! learned how mercurial actually worked :)
Attachment #8666819 - Flags: review?(s.kaspari)
(Assignee)

Updated

3 years ago
Attachment #8666645 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8666819 - Attachment is obsolete: true
Attachment #8666819 - Flags: review?(s.kaspari)
(Reporter)

Comment 10

3 years ago
(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)
(Assignee)

Comment 11

3 years ago
Created attachment 8666851 [details] [diff] [review]
patch_v3.diff

Yes, forgot to remove spaces.
Flags: needinfo?(shubhamkjain)
Attachment #8666851 - Flags: review?(s.kaspari)
(Reporter)

Comment 12

3 years ago
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+
(Assignee)

Comment 14

3 years ago
Thank you. I'll gradually improve my logging skills :)
(Assignee)

Comment 15

3 years ago
Created attachment 8667222 [details] [diff] [review]
patch_v4.diff
Attachment #8666851 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8667222 - Flags: review+
(Assignee)

Comment 16

3 years ago
the patch didn't work for Android 2.3 opt and debug, right?
(Reporter)

Comment 17

3 years ago
(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.
(Assignee)

Comment 18

3 years ago
Created attachment 8667846 [details] [diff] [review]
patch-v5.diff
Attachment #8667222 - Attachment is obsolete: true
(Assignee)

Comment 19

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

I've added range_list to ids.xml
Attachment #8667846 - Flags: review?(s.kaspari)
(Reporter)

Comment 20

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

Comment 21

3 years ago
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+
(Assignee)

Comment 22

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

Updated

3 years ago
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
(Assignee)

Comment 24

3 years ago
i don't really understand why the patch failed to apply. Can you please elaborate the reason?
Flags: needinfo?(shubhamkjain) → needinfo?(s.kaspari)
(Reporter)

Comment 25

3 years ago
(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)
(Assignee)

Comment 26

3 years ago
i still don't understand why i can't add 'range_list' to ids.xml.
Flags: needinfo?(s.kaspari)
(Reporter)

Comment 27

3 years ago
(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)
(Assignee)

Comment 28

3 years ago
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)
(Assignee)

Comment 29

3 years ago
Created attachment 8672452 [details] [diff] [review]
patch-v7.diff

I hope this patch works :)
Attachment #8667846 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8672452 - Flags: review?(s.kaspari)
(Reporter)

Comment 30

3 years ago
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+
(Reporter)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b310a8f86728
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.