Closed Bug 1122767 Opened 7 years ago Closed 6 years ago

Remove lockscreen code from SearchWidget

Categories

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

35 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: wesj, Assigned: ronak9896, Mentored)

References

Details

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

Attachments

(2 files, 2 obsolete files)

+++ 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
Mentor: wjohnston
Whiteboard: [good first bug][lang=java]
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
Ronak - that looks like it; Wesj, can you confirm?
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.
Flags: needinfo?(wjohnston)
Attached patch Patch_1 (obsolete) — Splinter Review
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.
Assignee: nobody → ronak9896
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!
Attachment #8554604 - Flags: review?(wjohnston)
Attachment #8554604 - Flags: feedback?(margaret.leibovic)
Michael- Roger that.I'll make sure to add flags next time 
Thank you
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).
Attachment #8554604 - Flags: feedback?(margaret.leibovic) → feedback+
Attached patch Bug1122767.patch (obsolete) — Splinter Review
Attachment #8557099 - Flags: review?(margaret.leibovic)
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
Attachment #8557099 - Flags: review?(margaret.leibovic) → review?(wjohnston)
Attachment #8557099 - Flags: review?(wjohnston) → review+
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!
Attachment #8554604 - Flags: review?(wjohnston) → review+
Flags: needinfo?(wjohnston)
This needs a Try run before someone can use checkin-needed
Hey Mark,

I am not sure if I understood what you mean by that.
(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
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?
Flags: needinfo?(michael.l.comella)
(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!
Flags: needinfo?(michael.l.comella) → needinfo?(ronak9896)
Flags: needinfo?(ronak9896)
Keywords: checkin-needed
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!
Flags: needinfo?(ronak9896)
Keywords: checkin-needed
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?
Flags: needinfo?(ronak9896) → needinfo?(wjohnston)
(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.
Flags: needinfo?(wjohnston)
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.
Attachment #8554604 - Attachment is obsolete: true
Attachment #8557099 - Attachment is obsolete: true
Attachment #8569866 - Flags: review?(wjohnston)
Attachment #8569866 - Flags: review?(michael.l.comella)
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?
Attachment #8569866 - Flags: review?(michael.l.comella)
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.
Attachment #8569866 - Flags: review?(wjohnston) → review+
I think this is final.We should mark it checkin-needed ?
Attachment #8571744 - Flags: review?(wjohnston)
The try run looked fine. Yep!
Keywords: checkin-needed
Comment on attachment 8571744 [details] [diff] [review]
patch1122767.diff

In the future, please just update the patch :)
Attachment #8571744 - Flags: review?(wjohnston)
https://hg.mozilla.org/integration/fx-team/rev/28141fe89b4c
Keywords: checkin-needed
Whiteboard: [good first bug][lang=java] → [good first bug][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/28141fe89b4c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=java][fixed-in-fx-team] → [good first bug][lang=java]
Target Milestone: --- → Firefox 39
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.