Add PageTitle class to canonicalize page names#738
Conversation
|
Is anyone able to review this? :) |
| */ | ||
| @NonNull | ||
| public String getPrefixedText() { | ||
| if (namespace.length() == 0) { |
There was a problem hiding this comment.
Replace with namespace.isEmpty()
There was a problem hiding this comment.
Thanks for the suggestion. Done in the modified commit.
Codecov Report
@@ Coverage Diff @@
## master #738 +/- ##
=========================================
+ Coverage 6.23% 6.83% +0.59%
=========================================
Files 93 94 +1
Lines 5033 5047 +14
Branches 455 456 +1
=========================================
+ Hits 314 345 +31
+ Misses 4693 4674 -19
- Partials 26 28 +2
Continue to review full report at Codecov.
|
misaochan
left a comment
misaochan
left a comment
There was a problem hiding this comment.
I finally had time to test this today (sorry for the long wait @whym ) and I seem to be having issues with loading appropriate-sized images in the media details pager view. If the picture is a landscape while the phone is held in portrait, previously it would resize the picture to an appropriate width, with blank area above and below. Currently it just crops part of it.
|
Also, one of the photos that I uploaded while testing this, https://commons.wikimedia.org/wiki/File:Korean_bbq_set.jpg , actually went through (can be viewed in browser), but does not display appropriately either in the main Contributions view or in the media details view. |
|
I couldn't reproduce the blank picture issue (#738 (comment)). Maybe it's specific to certain API levels? I didn't have the issue with API 23 (real device) and API 25 (emulator). If I understand it correctly, the crop issue (#738 (review)) does happen to me, too. However, I thought that the app behaved that way for some time. Doesn't it happen with the current master? If so, that might have to be filed as a new bug. I agree it's not ideal. Anyway, I have changed something that might help in the updated commit. |
|
I'm on a API 23 real device. The blank problem doesn't happen to certain pics, perhaps it's related to the file name chosen? Ah, I never noticed the crop issue before - I know prior to the hackathon it wasn't there. Will test master when I can. |
|
The crop issue does indeed occur on master as well. :/ Not sure when it was introduced. Will try and test this branch further afterwards. Not sure if the blank picture issue was preexisting either. |
|
I tested again and the blank issue persists, always for the most recent photo uploaded (so currently "korean bbq set" is displayed, but not the latest photo that I uploaded). Restarting the app fixes the problem. I'm testing on a Samsung Galaxy s7 with Android 7.0 (so API 24 and not 23, sorry for the confusion). |
|
Still no luck with reproducing it. (I'm using API 24 emulator.) Do you see any suspicious error messages before or after the frame is shown? |
|
Nothing more than the usual LeakCanary warnings. Do you think logs would help? |
|
Tested this again today (exact same API and device), and it appears to happen sporadically. Not all pictures cause this problem, but it happens at least 50% of the time for me. I can confirm that this does not happen with master. |
|
@whym I just realized that this bug does happen on master for me, and always after #667 occurs. So I think it's safe to say it wasn't caused by your PR. Sorry! (When I tested your PR vs master initially, by coincidence #667 did not occur on master, so this bug did not occur) Is this PR still merge-ready? Am happy to merge if it is. :) |
|
It's been rebased and is ready to be merged. (I have also added a few coding-style fixes but this should not affect the behavior.) |
4 participants
This is basically a refactoring. No effective change is intended (unless I missed something).