Added an onclick to the middle text of a Notification #1872
Conversation
Added on click to the "middle text" of a Notification item.
| } | ||
| }; | ||
|
|
||
| ss.setSpan(clickableSpan, 0, notificationText.length(), Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); |
There was a problem hiding this comment.
Could you add a comment explaining the magic number "0" or make it a constant with an informative name?
There was a problem hiding this comment.
Ok, I will add a comment.
Codecov Report
@@ Coverage Diff @@
## master #1872 +/- ##
=========================================
+ Coverage 3.61% 3.66% +0.04%
=========================================
Files 188 189 +1
Lines 9839 10041 +202
Branches 873 916 +43
=========================================
+ Hits 356 368 +12
- Misses 9457 9646 +189
- Partials 26 27 +1
Continue to review full report at Codecov.
|
Added comment to better explain method usage.
|
Testing the changes... sorry about the delay. |
|
The build looks fine but how do I manually test this? I'm not sure how to trigger a notification. How did you test this? |
|
Do you not already have any notifications in your Notifications activity (accessed via nav bar)? Usually you would have one just as a welcome, even if the acc is new. |
|
I tested adding a Log function to the onclick then looked at the output in the logcat. If I am not mistaken I also tested modifying the onclick to open the notification. |
|
@misaochan Unfortunately no, I don't have the welcome notification :( |
|
@albendz What's your Commons username? One of us can send you a user talk message. :) |
|
My user name is abendz. I will eagerly await the notification! |
|
Sent :) |
|
Unfortunately it's not coming through the app. At this point we may want to ask someone else to test :( |
|
No worries. However, could you please create a separate issue for your notifications problem, with a screenshot of what you see in the Notifications activity? As this might indicate an issue with notifications in general. Thanks! (Edit: Just to clarify, we don't mean notifications in the Android sense. "Notifications" is an activity that is accessed via nav drawer, although we will soon be adding an icon to the main screen. Not sure if that was made clear. :)) |
|
@albendz can you see your notifications from web interface of commons? If so we need to find out why your notifictions are not fetched. I will be testing it, though |
|
@neslihanturan I got the email about the talk message and it is showing in my notification history on the web. Thank you for picking up the testing. |
|
Alright, I'll create a second account and start talking to myself to generate notifications and get those logs :P |
|
My problem: "You must be logged in."
|
|
Oh! You need to be on a prod build variant, not beta. :) |
|
I had tried on prod and wasn't getting any logs so I tried again using the beta website and the master branch of code to see if I could get notification and still wasn't but I was seeing this log. So, I retried this in master+beta and confirmed the web notifications are coming through but I'm still not able to get notifications on the phone. I couldn't see logs in the prod version. |
|
@albendz Can you confirm what is the last commit in your master. We made a few fixes around authentication which was merged to master yesterday. Notifications should work both on beta and prod variants. For prod you can use the following gradle task to make it debuggable: |
|
This is the last master commit I have: I'm a little confused about why the PR uses prod and why master uses beta. |
|
It's not that the PR uses prod. Some of the features in the app (eg. wiki data edits, upload count) work only on prod variant as the corresponding API for the beta environment isn't available. As far as notifications are concerned it should work both on beta and prod variant. Beta: Commons Web Url: https://commons.wikimedia.beta.wmflabs.org Commons Web URL: http://commons.wikimedia.org If you check out our app's |
|
Additionally, the reason why we have the beta build variant to begin with, is to assist with testing. Testing certain features requires uploading a lot of pictures, and people don't always have pictures that are "Commons-worthy". The beta build variant lets you upload photos to the beta Commons server, which isn't the same as the REAL Commons server. So you can upload nonsense without worrying about spamming Commons. :) Basically, use the beta build variant only when you really need to upload test pictures and don't want to pollute Commons. @maskaravivek While notifications should work on both technically, I don't think cross-notifications is possible. So if I sent @albendz a message on the real Commons server, she wouldn't see anything in beta AFAIK? |
@albendz Could we get a screencast of what happens when you try it, please? You can create a separate issue if you like. The problem now is not so much the testing of this PR, but rather that if notifications aren't working for someone, we need to figure out why. |
|
Sure, I'll get a recording. Beta or prod? |
|
@neslihanturan My issue aside, are you still working on testing this? Just want to make sure it doesn't get lost in the other issue. |
There was a problem hiding this comment.
Tested tutorial, login, home, notifications, about page.
2.8.1-debug-pr-1872~bda89a0b on Samsung Galaxy S6 with API level 25
2.8.1-debug-pr-1872~bda89a0b on Galaxy Nexus (emulator) with API level 28
Results
- Pages all okay
- All my notifications show up, in the correct order
- Notifications which don't have a url:
- Icon gives tap animation
- Middle text does nothing
- Date gives tap animation
- Show less/more works
- Notifications which do have a url:
- Icon gives tap animation and opens relevant page
- Middle text opens relevant page
- Date opens relevant page
- Show less/more works
I'm happy to merge as everything is working. However, I'd suggest changes for the future as:
- Have clicking on the middle text also show animation for consistency (also pressing and holding should make row darker shade like others)
- Do something better with show more/show less:
- As the target is small and the middle text link surrounds it, it's annoying to accidentally open the browser. Not sure how to fix this one, however I think having the functionality of clicking the text opening the browser is worth it.
- All the times it's turned up for me it's not been helpful - with the 'show more' taking up more space than if it wasn't there. Maybe need to be less aggressive with it? item_notification.xml
A solution that might solve all of these issues is doing away with the "show more", and having the entire notification row be a link. Having a longer trimLength (I'm thinking like 280 chars) with an ellipsis if it's somehow over that should be okay. The user can view the commons website for the entire notification text if it's really long, if they're clicking through to the website anyways.
I've put these into two new issues, #1980 and #1981 to continue discussion.
Enhancement issue #1862
Used a SpannableString to setup a ClickableSpan in the middle text of a Notification item so it works the same as when you click at the date in the right side or in the icon at the left.