1060056 - Rename MainActivity to SearchActivity

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[:Margaret Leibovic]

It's confusing that there's an Activity called "MainActivity" in the tree, especially since the "main activity" of Fennec is BrowserApp.

Comment 1

[Projjol [:dorsatum]]

Hi, I'd like to work on this. I have a working firefox for android build, however to get started with this bug, where should I start looking?

Comment 2

[Mike Hoye [:mhoye]]

Hi, Projjol. Thanks for your interest! Margaret, can you point Projjol to the part of the source to get them started?

Comment 3

[:Margaret Leibovic]

Hi Projjol! You should look for the MainActivity class in our search activity code:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/MainActivity.java

You'll want to rename this class name (and the file name), and then update any reference to it in the tree.

However, our workflow for this search activity part of the tree is a bit different than just writing a patch directly in mozilla-central, since we also have a fennec-search github repo that allows us to work on the search activity independently of the rest of fennec.

So, you should clone this repo, and make a pull request with your changes there:
https://github.com/mozilla/fennec-search

Then I can help you generate a patch for us to land on mozilla-central as well.

For more details, see this mailing list post:
https://mail.mozilla.org/pipermail/mobile-firefox-dev/2014-August/000844.html

Comment 4

[shivendrayadav2012]

hi guys,the discussion seems very interesting.i would like to add my android experience to this .
can i join from the links provided.

Comment 5

[Projjol [:dorsatum]]

Hi Margaret, thank you for your help! I've edited the file in the forked version of Fennec and in the local tree that I have on my machine (haven't committed changes to the local tree though). Could I contact you on the irc?

Comment 6

[:Margaret Leibovic]

(In reply to shivendrayadav2012 from comment #4)
> hi guys,the discussion seems very interesting.i would like to add my android
> experience to this .
> can i join from the links provided.

Projjol is already working on this, so you may be more interested in picking up a different bug. You're welcome to comment on any bugs to give feedback, but there's not too much to discuss in this bug, since it's just about renaming a class :)

(In reply to Projjol [:dorsatum] from comment #5)
> Hi Margaret, thank you for your help! I've edited the file in the forked
> version of Fennec and in the local tree that I have on my machine (haven't
> committed changes to the local tree though). Could I contact you on the irc?

Sure, feel free to ping me in #mobile (or just ask questions there in general, there are plenty of friendly people there who would love to help you).

Did you make your changes to the fennec-search repo or to mozilla-central? Once you've made the changes in fennec-search, you can run `grunt export --tree <path to your mozilla-central>` to export the changes to mozilla-central. Then you can follow these steps to generate a patch as you normally would for any patch to mozilla-central:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 7

[Projjol [:dorsatum]]

Sorry for being inactive! I've made changes to the file, firstly, I've renamed the file to SearchActivity, and within this file, I've changed instances of MainActivity to SearchActivity. This is the output of hg diff post the grunt command : https://pastebin.mozilla.org/6537208

This diff contains changes other than the one I made. How should I proceed?

Comment 8

[:Margaret Leibovic]

(In reply to Projjol [:dorsatum] from comment #7)
> Sorry for being inactive! I've made changes to the file, firstly, I've
> renamed the file to SearchActivity, and within this file, I've changed
> instances of MainActivity to SearchActivity. This is the output of hg diff
> post the grunt command : https://pastebin.mozilla.org/6537208
> 
> This diff contains changes other than the one I made. How should I proceed?

It looks like you just need to make sure that your fennec-search repo and your mozilla-central tree are both up to date. Looking at your patch, it looks like you generated it after bug 1066033 was merged into fennec-search, but perhaps the change wasn't on mozilla-central yet. If something like that happens again, you should be able to look at blame to see what the status of the bug is.

In any case, if you update both your trees then generate the patch again, you should attach it to this bug and request review from me. 

You should also generate a pull request for fennec-search, and I will merge that when I land the patch for mozilla-central.

Comment 9

[Projjol [:dorsatum]]

Attachment: SearchActivity.patch

Comment 10

[:Margaret Leibovic]

Comment on attachment 8492698 [details] [diff] [review]
SearchActivity.patch

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

It looks like this patch is missing the actual rename of the file. The `grunt export` command makes this tricky, and I failed to explain that before.

What you should do is run `hg rename mobile/android/search/java/org/mozilla/search/MainActivity.java mobile/android/search/java/org/mozilla/search/SearchActivity.java` then export your changes. I just commented in the PR to explain what you need to do there as well.

Here's a search that shows where MainActivity is used:
http://mxr.mozilla.org/mozilla-central/search?string=mainactivity&find=%2Fmobile%2Fandroid%2F&findi=&filter=^%5B^\0%5D*%24&hitlimit=&tree=mozilla-central

You'll need to make sure to rename these things as well.

Comment 11

[:Margaret Leibovic]

Also, please make sure you build and test this! The build would have failed with this current patch applied.

Comment 12

[:Margaret Leibovic]

Hey Projjol, I wanted to let you know that we recently decided to stop using this fennec-search github repo for development, since it's starting to cause more headaches than it's solving (like needing to generate two different patches here!).

So, you can just write a patch for m-c like you normally would for any other part of Firefox, and I'll happily review it :)

The patch you have in here is on the right track, you just need to `hg mv` the file and update the class references, as I mentioned in other comments above.

Comment 13

[Projjol [:dorsatum]]

Attachment: SearchActivity.patch

Built m-c with the patch in it and it went off smoothly.

Comment 14

[:Margaret Leibovic]

Comment on attachment 8494935 [details] [diff] [review]
SearchActivity.patch

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

This is looking good, but it failed to apply when I tried to test it out. Can you update your tree and rebase? Thanks!

Comment 15

[Projjol [:dorsatum]]

Attachment: SearchActivity.patch

Updated m-c, applied the patch and built with it.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8495763 [details] [diff] [review]
SearchActivity.patch

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

Great! I can land this for you. Thanks for the patch!

Comment 17

[:Margaret Leibovic]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26f99dd8b652

Comment 19

[Sudhir]

Can I take up this issue?

Comment 20

[Projjol [:dorsatum]]

Hi Sudhir,this issue has been fixed :)