Closed
Bug 1122767
Opened 10 years ago
Closed 10 years ago
Remove lockscreen code from SearchWidget
Categories
(Firefox for Android Graveyard :: Settings and Preferences, defect)
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)
1.95 KB,
patch
|
wesj
:
review+
|
Details | Diff | Splinter Review |
1.17 KB,
patch
|
Details | Diff | Splinter Review |
+++ 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
Updated•10 years ago
|
Mentor: wjohnston
Whiteboard: [good first bug][lang=java]
Assignee | ||
Comment 1•10 years ago
|
||
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•10 years ago
|
||
Ronak - that looks like it; Wesj, can you confirm?
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
Michael- Roger that.I'll make sure to add flags next time
Thank you
Comment 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8557099 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 9•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Attachment #8557099 -
Flags: review?(margaret.leibovic) → review?(wjohnston)
Reporter | ||
Updated•10 years ago
|
Attachment #8557099 -
Flags: review?(wjohnston) → review+
Reporter | ||
Comment 10•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(wjohnston)
Comment 11•10 years ago
|
||
This needs a Try run before someone can use checkin-needed
Assignee | ||
Comment 12•10 years ago
|
||
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
Assignee | ||
Comment 14•10 years ago
|
||
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?
Updated•10 years ago
|
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)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(ronak9896)
Keywords: checkin-needed
Comment 16•10 years ago
|
||
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
Assignee | ||
Comment 17•10 years ago
|
||
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)
Assignee | ||
Comment 19•10 years ago
|
||
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)
Reporter | ||
Comment 21•10 years ago
|
||
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+
Assignee | ||
Comment 22•10 years ago
|
||
I think this is final.We should mark it checkin-needed ?
Attachment #8571744 -
Flags: review?(wjohnston)
Reporter | ||
Comment 23•10 years ago
|
||
The try run looked fine. Yep!
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 24•10 years ago
|
||
Comment on attachment 8571744 [details] [diff] [review]
patch1122767.diff
In the future, please just update the patch :)
Attachment #8571744 -
Flags: review?(wjohnston)
Comment 25•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [good first bug][lang=java] → [good first bug][lang=java][fixed-in-fx-team]
Comment 26•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=java][fixed-in-fx-team] → [good first bug][lang=java]
Target Milestone: --- → Firefox 39
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•