Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Added Caption and UI for depicts #2970

Open
wants to merge 206 commits into
base: backend-overhaul
from

Conversation

@vanshikaarora
Copy link
Collaborator

vanshikaarora commented May 28, 2019

Description (required)

Fixes #2297 Submit caption to Structured Data and #2097 Structured Commons "depicts" property

What changes did you make and why?

  • Modified UI for Upload Activity Step 1
  • Created layout for depicts same as Category
VitalyVPinchuk and others added 4 commits May 21, 2019
* Fix duplicate param information (#2515)

* Bug fix issue #2476 (#2526)

* Added wikidataEntityID in all db versions, handled db.execSql via method runQuery

* Versioning and changelog for v2.10.2 (#2531)

* Update changelog.md

* Versioning for v2.10.2

* Update changelog.md

* Bugfix/issue 2580 (#2584)

* Corrected string placedholders in certain string files

* Corrected string placedholders in certain string files[Bug fix #2580]

* Bug Fix #2585 (#2647)

* Bug Fix #2585
* Added null checks on view in SearchImageFragment when updating views from external sources
* Disposed the disposables in SearchActivity and SearchImageFragment when no longer in active lifecycle

* use FragmentUtils to verify fragment active state

* Bug Fix issue #2648 (#2678)

* Bug Fix issue #2648
* Handled external storage permission before file download

* * Removed redudant check for permission in MediaDetailPagerFragment (Dexter already does that)
* Removed duplicate code in PermissionUtil$checkPermissionsAndPerformAction, used the existing function with conditional extra parameters

* string name typo correction

* BugFix issue #2652 (#2706)

* Addded null check on bookmark before operating on it

* BugFix issue #2711 (#2712)

* Added null checks in OkHttpJsonApiClient$searchImages MwQueryResponse

* BugFix #2718 (#2719)

* Handled null auth cookies

* Fix #2791: NPE when nominating for deletion and leaving screen (#2792)

* Bug Fix issue #2789 (#2790)

* Handled Illegal State Exception for non existent appropriate view parents in ViewUtils$showShortSnackbar

* BugFix #2720 (#2831)

BugFix deprecated licenes #2720

* ui fixes, wip, upload

* *Issue #2886, BugFix #2832[wip]
* updated UploadActivity code
* modified ui
* Updated UploadPresenterTest

* * updated interfaces names to follow names suffixed with Contract
* added test cases

* card view elevation

* view pager disabled swipe

* bug fix, duplicate image

* used existing non-swipable view pager

* Avoid image view resize with keyboard, added adjustPan and stateVisible as softinputMode for UploadActivity

* retain UploadBaseFragment instances on orientation changes

* * Added test cases for UploadMediaPresenter
* Injected io and main thread schedulers

* categories presenter test cased wip

* Added CategoriesPresenter test

* * Added the logic to show open map (with to be uploaded image's coordinates while uploading image)

* codacy suggested changes * added java docs

* Added travis_wait fot android-wait-for-emulator

* ranamed interface onResponseCallback to Callback

* * Added api to delete picture in UploadModel
* cleanUp in UploadModel. once upload has been initiated
* Removed unused methods from UploadModel and the corresponding test class

* * Added tests for UploadPresenter
* Travis suggested changes
* Addded copy previous title and description

* * Made the upload add descriptions visible when keyboard visible
* add description request focus only when user manually requests it

* Added JavaDocs, review suggested changes

* Fix dagger injection

* use DialogUtil to show info in descriptions

* use activity context for DialogUtil

* Minor changes
Copy link

pullrequest bot left a comment

A review job has been created and sent to the PullRequest network.


@vanshikaarora you can click here to see the review status or cancel the code review job.

@vanshikaarora vanshikaarora changed the title Added Caption and UI for depicts [WIP] Added Caption and UI for depicts May 28, 2019
@@ -59,7 +59,7 @@
<string name="add_set_name_toast">Please provide a name for this set</string>
<string name="provider_modifications">Modifications</string>
<string name="menu_upload_single">Upload</string>
<string name="categories_search_text_hint">Search categories</string>

This comment has been minimized.

Copy link
@madhurgupta10

madhurgupta10 May 28, 2019

Contributor

@vanshikaarora looks like removing this line is causing the build to fail, the string categories_search_text_hint is already in use at activity_upload_categories.xml

This comment has been minimized.

Copy link
@nicolas-raoul

nicolas-raoul May 29, 2019

Member

Thanks for catching that!
Indeed both are needed, the app will have both categories and "depicts".

This comment has been minimized.

Copy link
@vanshikaarora

vanshikaarora May 29, 2019

Author Collaborator

Thanks for checking that @madhurgupta10 :)

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 1, 2019

Codecov Report

Merging #2970 into refactor_uploads will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           refactor_uploads   #2970      +/-   ##
===================================================
- Coverage              4.36%   4.36%   -0.01%     
===================================================
  Files                   257     259       +2     
  Lines                 12358   12380      +22     
  Branches               1059    1059              
===================================================
  Hits                    540     540              
- Misses                11780   11802      +22     
  Partials                 38      38
Impacted Files Coverage Δ
.../java/fr/free/nrw/commons/upload/UploadModule.java 0% <ø> (ø) ⬆️
.../fr/free/nrw/commons/di/FragmentBuilderModule.java 0% <ø> (ø) ⬆️
...ava/fr/free/nrw/commons/upload/UploadActivity.java 0% <0%> (ø) ⬆️
...e/nrw/commons/upload/depicts/DepictsPresenter.java 0% <0%> (ø)
...ee/nrw/commons/upload/depicts/DepictsFragment.java 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7712f0...79d37ff. Read the comment docs.

vanshikaarora and others added 9 commits Jun 2, 2019
Copy link
Member

nicolas-raoul left a comment

Also, there is still a fr.free.nrw.commons_2019.04.15_22.10.li file, is it needed?

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@vanshikaarora

This comment has been minimized.

Copy link
Collaborator Author

vanshikaarora commented Aug 19, 2019

Also, there is still a fr.free.nrw.commons_2019.04.15_22.10.li file, is it needed?

No, not needed.
I'll remove them all


try {
LinkedTreeMap statements = (LinkedTreeMap) commonsWikibaseItem.getStatements();
ArrayList<LinkedTreeMap> p180ItemList = (ArrayList<LinkedTreeMap>) statements.get("P180");

This comment has been minimized.

Copy link
@nicolas-raoul

nicolas-raoul Aug 21, 2019

Member

The Wikidata property "P18" is already hardcoded (in Nearby), so hardcoding the Commons property "P180" too is not a revolting thing. The difference is that other parts of the app use the Commons beta server, while Nearby does not use any Wikidata beta server. I suggest leaving that as an enhancement: #3135

*/

public void replaceTitlesWithCaptions(String displayTitle, int i) {
compositeDisposable.add(mediaClient.getCaptionByFilename("File:" + displayTitle+ ".jpg")

This comment has been minimized.

Copy link
@nicolas-raoul

nicolas-raoul Aug 21, 2019

Member

(see my comment 10 lines above)

app/src/main/java/fr/free/nrw/commons/Media.java Outdated Show resolved Hide resolved
app/src/main/java/fr/free/nrw/commons/Media.java Outdated Show resolved Hide resolved
app/src/main/java/fr/free/nrw/commons/Media.java Outdated Show resolved Hide resolved
*/

public void replaceTitlesWithCaptions(String displayTitle, int i) {
compositeDisposable.add(mediaClient.getCaptionByFilename("File:" + displayTitle+ ".jpg")

This comment has been minimized.

Copy link
@nicolas-raoul

nicolas-raoul Aug 21, 2019

Member

(after solving this, please apply same resolution at DepictedImagesPresenter.java)

@nicolas-raoul

This comment has been minimized.

Copy link
Member

nicolas-raoul commented Aug 21, 2019

@vanshikaarora

This comment has been minimized.

Copy link
Collaborator Author

vanshikaarora commented Aug 21, 2019

The Media.java does not contain what we need?
Can't we specify (in the URL) what return values we want?

Yes, I am currently working on it

Copy link
Member

nicolas-raoul left a comment

Many localization files have changes, this can probably be solved by rebasing.
Please remove captures/fr.free.nrw.commons_2019.04.15_22.10.li.

@@ -186,6 +227,10 @@ public static Media from(MwQueryPage page) {
return media;
}

public String getPageId() {

This comment has been minimized.

Copy link
@nicolas-raoul

nicolas-raoul Aug 23, 2019

Member

In the PR it appears as an addition though:
Screenshot from 2019-08-23 11-51-55
If not needed, feel free to remove it.

this.csrfTokenClient = csrfTokenClient;
}

public Observable<Boolean> postEditEntity(String fileEntityId, String data) {

This comment has been minimized.

Copy link
@nicolas-raoul

nicolas-raoul Aug 23, 2019

Member

javadoc on methods

This comment has been minimized.

Copy link
@vanshikaarora

vanshikaarora Aug 23, 2019

Author Collaborator

@nicolas-raoul I hahve not much sure about this class, it wasn't added by me it was a result of merge with VitalyPinchuk's branch

String baseUrl = "https://upload.wikimedia.org/wikipedia/commons/thumb/";
title = title.replace(" ", "_");
if (!title.endsWith(".jpg")){
title+=".jpg";

This comment has been minimized.

Copy link
@nicolas-raoul

nicolas-raoul Aug 26, 2019

Member

Can this one be solved too?

Copy link
Member

nicolas-raoul left a comment

It is getting very good :-)

private String prettyCaption(Media media) {
String caption = media.getCaption().trim();
if (caption.equals("")) {
return getString(R.string.detail_caption_empty);

This comment has been minimized.

Copy link
@nicolas-raoul

nicolas-raoul Aug 26, 2019

Member

Is this solved? Is an empty row shown in some cases?

@vanshikaarora

This comment has been minimized.

Copy link
Collaborator Author

vanshikaarora commented Aug 26, 2019

Is this solved? Is an empty row shown in some cases?

Yes, the issue is solved. Empty row is not shown in case of no captions

} else {
depictionsRecyclerView.setLayoutManager(new GridLayoutManager(getContext(), 2));
}
ArrayList<DepictedItem> items = new ArrayList<>();

This comment has been minimized.

Copy link
@nicolas-raoul

nicolas-raoul Aug 27, 2019

Member

Is it taken care of now?
CC @ashishkumar0207

int firstVisibleItemPosition=0;
if(layoutManager instanceof GridLayoutManager){
firstVisibleItemPosition=((GridLayoutManager) layoutManager).findFirstVisibleItemPosition();
}else{

This comment has been minimized.

Copy link
@nicolas-raoul

nicolas-raoul Aug 27, 2019

Member

space before and after = and else in the few lines above and below here.


import static android.view.View.GONE;
import static android.view.View.VISIBLE;

This comment has been minimized.

Copy link
@ashishkumar468

ashishkumar468 Aug 27, 2019

Collaborator

Javadoc

} else {
depictionsRecyclerView.setLayoutManager(new GridLayoutManager(getContext(), 2));
}
ArrayList<DepictedItem> items = new ArrayList<>();

This comment has been minimized.

Copy link
@ashishkumar468

ashishkumar468 Aug 27, 2019

Collaborator

IMO, this is not required, the arraylist must be unused now, right ?

this.query = query;
bottomProgressBar.setVisibility(View.VISIBLE);
progressBar.setVisibility(GONE);
compositeDisposable.add(depictsClient.searchForDepictions(query, 25, queryList.size())

This comment has been minimized.

Copy link
@ashishkumar468

ashishkumar468 Aug 27, 2019

Collaborator

Where have you cleared the composite disposable.

This comment has been minimized.

Copy link
@vanshikaarora

vanshikaarora Aug 27, 2019

Author Collaborator

is present in the recent commit

import static android.view.View.GONE;
import static android.view.View.VISIBLE;

public class SearchDepictionsFragment extends CommonsDaggerSupportFragment {

This comment has been minimized.

Copy link
@ashishkumar468

ashishkumar468 Aug 27, 2019

Collaborator

Why is this class not following MVP ?

This comment has been minimized.

Copy link
@vanshikaarora

vanshikaarora Aug 27, 2019

Author Collaborator

@ashishkumar468 I think this is some previous commit. Currently this class does follows MVP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.