1122767 - Remove lockscreen code from SearchWidget

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Description

[Wesley Johnston (:wesj)]

+++ This bug was initially created as a clone of Bug #1121823 +++

We still have code in SearchWidget.java to handle showing over the keyguard (lockscreen). We don't support that any more, so we should remove it.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/SearchWidget.java#108

Comment 1

[ronak khandelwal]

Hey,
I am new to open source and was wondering if I could get some more info on this bug that'd be great.From what read I think we basically just have to remove the code segment and all its calls?. My most sincere apologies if I ask any stupid questions.
Thank you

Comment 2

[Mike Hoye [:mhoye]]

Ronak - that looks like it; Wesj, can you confirm?

Comment 3

[:Margaret Leibovic]

Welcome, ronak!

Yes, it looks like we just need to remove that part of the code and any related calls. wesj can jump in to correct me if there's more to it than that :)

Do you have a build environment set up? If not, you should follow the directions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

Feel free to hop in #mobile on irc.mozilla.org for any questions about getting a build working or how to start working on this bug.

Comment 4

[ronak khandelwal]

Attachment: Patch_1

Hey,
This is my first patch. Do tell me if there's something wrong also I am not sure about the testing scenario as to how to test the changes I built the package again and it completed succesfully.
Thanks.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8554604 [details] [diff] [review]
Patch_1

Hey, Ronak!

Good to see you here too! :P

When you upload a patch for a mentor to look at, it's best practice to add some review (r?) or feedback (f?) flags so they get a notification (rather than an easily lost email).

To do this, go to the attachment's details page, add a question mark next to the flag.

Typically review flags are used for patches you think are final and feedback flags are for non-final patches you have more work to do on.

I've added the flags for you here but don't forget to do so next time!

Comment 6

[ronak khandelwal]

Michael- Roger that.I'll make sure to add flags next time 
Thank you

Comment 7

[:Margaret Leibovic]

Comment on attachment 8554604 [details] [diff] [review]
Patch_1

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

Thanks for the patch! I have a few pieces of feedback. You can address these and then upload a new version of the patch for review.

::: mobile/android/search/java/org/mozilla/search/SearchWidget.java
@@ -113,3 @@
>          final RemoteViews views;
> -        if (isKeyguard) {
> -            views = new RemoteViews(context.getPackageName(), R.layout.keyguard_widget);

You should also remove this keyguard_widget layout file:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/keyguard_widget.xml

@@ +110,1 @@
>              views = new RemoteViews(context.getPackageName(), R.layout.search_widget);

There's no need to declare this variable separately from setting it now, so you can combine these into one line.

@@ +110,3 @@
>              views = new RemoteViews(context.getPackageName(), R.layout.search_widget);
>              addClickIntent(context, views, R.id.search_button, ACTION_LAUNCH_SEARCH);
> +        

Nit: Please remove the trailing whitespace in this patch (for the future, it's good to tweak your text editor settings to not add trailing whitespace).

Comment 8

[ronak khandelwal]

Attachment: Bug1122767.patch

Comment 9

[ronak khandelwal]

Hey,

Please review the patch I've made all the required changes.Also I would be very grateful if you could suggest me another bug to work on .
Thank you

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8554604 [details] [diff] [review]
Patch_1

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

Thanks for this!

If you're still looking for a new bug, https://bugzilla.mozilla.org/show_bug.cgi?id=1076966 seems fun!

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

This needs a Try run before someone can use checkin-needed

Comment 12

[ronak khandelwal]

Hey Mark,

I am not sure if I understood what you mean by that.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to ronak khandelwal from comment #12)
> Hey Mark,
> 
> I am not sure if I understood what you mean by that.

Hi, Ronak. A Try run is a push to our test servers - I pushed and you can find the results at the following link:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31ffcec22ebd

When the results are green, you can add the "checkin-needed" keyword [1] to the top of the page to get the patch checked in - we need a green try run for all checkins via "checkin-needed".

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 14

[ronak khandelwal]

Hey Michael ,

Long time no see, Thank you ,again . I was wondering if you would vouch for me on mozillians.

Thank you anyways


p.s: I think we got a green try run there should I change the flag?

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to ronak khandelwal from comment #14)
> Long time no see, Thank you ,again . I was wondering if you would vouch for
> me on mozillians.

Sure - what's your email address? Is it your bugmail address? If not, I'd recommend emailing it to my bugmail address (rather than posting it here).

> p.s: I think we got a green try run there should I change the flag?

Looks good to me! Add "checkin-needed" when you're ready!

Thanks, ronak!

Comment 16

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

renamed 1122767 -> Bug1122767.patch
applying Bug1122767.patch
patching file mobile/android/search/java/org/mozilla/search/SearchWidget.java
Hunk #1 FAILED at 96
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/search/java/org/mozilla/search/SearchWidget.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

could you take a look, thanks!

Comment 17

[ronak khandelwal]

Hey,

From what I understood from http://mercurial.selenic.com/wiki/HandlingRejects  is I should just pull the latest changes from the server and then update the file again?

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to ronak khandelwal from comment #17)
> From what I understood from
> http://mercurial.selenic.com/wiki/HandlingRejects  is I should just pull the
> latest changes from the server and then update the file again?

Correct – when you pull the changes from the server (`hg pull`; make sure you `hg qpop -a` to pop your patches first!), then reapply your patch (`hg qpush`), you will likely receive output similar to comment 16. This means the code underneath has changed and so the patch merging software doesn't know how to cleanly apply the patch. You will receive a .rej file for each failed merge as output, which you need to combine with the file in your working directory. If you look online, the utility "wiggle" can help with this, but if the change is small, doing it by hand should work fine.

Comment 19

[ronak khandelwal]

Attachment: final1122767.diff

Sorry it took me so long to get this one right. I had to download the whole repo again for the tree was broken in my local repo(strange?).
Thank You.

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8569866 [details] [diff] [review]
final1122767.diff

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

Clearing review as I am unfamiliar with the code.

Don't forget a commit message for you patch! It should be in the form, "Bug # - <what-i-fixed>".

(In reply to ronak khandelwal from comment #19)
> Sorry it took me so long to get this one right. I had to download the whole
> repo again for the tree was broken in my local repo(strange?).

Version control can be tricky sometimes - perhaps you had some patches applied or made a permanent commit (rather than creating a patch to add to the queue) when trying to pull?

Comment 21

[Wesley Johnston (:wesj)]

Comment on attachment 8569866 [details] [diff] [review]
final1122767.diff

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

Looks good to me! Thanks!

::: mobile/android/search/java/org/mozilla/search/SearchWidget.java
@@ +102,5 @@
>  
>      // Utility to create the view for this widget and attach any event listeners to it
>      private void addView(final AppWidgetManager manager, final Context context, final int id, final Bundle options) {
> +        final RemoteViews views = new RemoteViews(context.getPackageName(), R.layout.search_widget);
> +        

There's some extra white space here.

Comment 22

[ronak khandelwal]

Attachment: patch1122767.diff

I think this is final.We should mark it checkin-needed ?

Comment 23

[Wesley Johnston (:wesj)]

The try run looked fine. Yep!

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8571744 [details] [diff] [review]
patch1122767.diff

In the future, please just update the patch :)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28141fe89b4c