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

GSoC 2019 Deliverable: Local File Transfer #1276

Merged
merged 144 commits into from
Aug 15, 2019
Merged

Conversation

Aditya-Sood
Copy link
Contributor

Using the WiFi P2P API, the app can now be used to transfer ZIM files (from the Library) from one device to another.
Partially solves #1136

Requirements:

  • The API requires (coarse) location permissions on Oreo and higher API levels
  • Both sender and receiver devices must have the kiwix app

Steps for usage:

  1. Sender: Select the files to be transferred & tap on share icon

  1. Sender: Tap on 'Send to nearby device' icon

Opens a screen like so on the sender device:

  1. Receiver: Tap on 'Get content from nearby device' icon

Opens a screen like so on the receiver device:

  1. Both: Tap on search icon to locate the other device (& grant the permissions requested if any). (Note that only the sender device shows list of files to be transferred before connection is completed).

  1. Sender: Tap on the name of the destination device

  2. Sender: Confirm receiver device

  1. Both: File transfer progress is displayed
    Sender device:

Receiver device:

  1. Activity closes once file transfer is complete

For some reason the files don't show up in the 'Library', but you can verify that they have been transferred by looking at the kiwix folder manually. This wasn't an issue with the previously used java code (from before merging develop: relevant commit), so I'm guessing there's an issue with the kotlin port.

The activity can now be used to send one selected ZIM file to a nearby device
(Cleanup and refactoring yet to be done)
Instead of using a service, so that the progress can be visually displayed
- During handshake, sender device informs the receiver of:
	1) Total no. of files
	2) Name of files to be sent
- The file name is now being used instead of "temp.zim" to save the received file in the 'KiwixWifi' directory
All selected files (not just the first one) are trasferred to the receiving device
- File receiver device can not initiate connection anymore
- Activity closes on both devices once transfer is complete
… folder

Location obtained from the SharedPreferenceUtil
White icon tint was corrected in the commit before the previous one
Instead of the general 'Wireless' settings page, incase the wifi hasn't been turned on for file transfer
…transferred

Next step - Use this list to display progress of transfer
Fields: File name and transfer status (to be sent, sending, sent)
3 possible states: To be sent (wait icon) -> sending (progress bar) -> sent (check icon)
Best guess - Calling disconnect after sending the last file was somehow corrupting the output stream to the server. Replacing with Toast (or just finish()) has corrected it.
- Separate device disconnection to separate method
- Override back button
For disconnecting a connection to an erroneous peer device
Device name for WiFi Direct is supposed to be changed manually (from the advanced wifi settings) by the user.

This solution used reflection to access the hidden method used by the system to perform the same. However use of non-SDK interfaces (fields/methods accessed using reflection) will be restricted and subsequently lead to exceptions, hence the feature needs to be removed.

For more: developer.android.com/distribute/best-practices/develop/restrictions-non-sdk-interfaces
Unnecessary comments, todos, variables, imports, methods
- Add title to activity
- Divide screen space between list of available peers and files for transfer
Comprehensive refactoring of DeviceListFragment, includes:
- Shifting all async-tasks to separate files
- Exceptions in the async-tasks are now handled by showing a toast and closing the LocalFileTransferActivity
- Added a new 'Error' status for file items
- If a file has error status after transfer, then an error icon is shown for it TransferProgressFragment
- Documentation and other additions/changes
Functionality has already been replaced by the SenderDeviceAsyncTask
Wasn't ignoring some .idea files
The 'Get zim from nearby device' option now only shows in 'Device' tab of library
Keeping with the I in SOLID, WDM now uses a plain Activity instead of LocalFileTransferActivity specifically
Removed extra methods + refactor
@Aditya-Sood
Copy link
Contributor Author

Aditya-Sood commented Aug 13, 2019

@mhutti1

The receive icon should imo only be on one of the tabs. I am not sure which makes most sense. Perhaps the device tab.

I have restricted it to the 'Device' tab as suggested

Could you also rebase this so that it has the changes to remove duplicate library entries that have been merged.

I merged develop into the branch

Perhaps we could also add a send button to the selected menu to allow easier sending.
Finally If we are to educate users about this feature how would you propose going about doing it?

If it feels non-intuitive to send from the 'share' option in the contextual menu, maybe I should create a separate point of entry for the whole module?
Like a new activity where the user chooses between being a sender and a receiver
(The receiver goes directly to LFTA, while the sender gets a file picker before going on to LFTA)

I was thinking of something like this with the nav-drawer UI (like a separate nav-drawer entry point), because this would make the flow pretty clear to the user. Do you have anything in mind?

@Aditya-Sood
Copy link
Contributor Author

Thanks @macgills! I've merged develop and fixed the changes
Love the pre-commit hook - nips it in the bud :)

@macgills
Copy link
Contributor

If it feels non-intuitive to send from the 'share' option in the contextual menu, maybe I should create a separate point of entry for the whole module?

It feels fine to me to have a send icon(beside share/delete) in the actionMode, it is better than piggybacking on the share intent anyhow.
Lets not make any changes this late in the game though. How about a ticket "Improve LocalTransfer UX"?

@Aditya-Sood
Copy link
Contributor Author

@macgills @mhutti1

It feels fine to me to have a send icon(beside share/delete) in the actionMode, it is better than piggybacking on the share intent anyhow.

Do I make this change now or in a new ticket?

@macgills
Copy link
Contributor

Lets not make any changes this late in the game though. How about a ticket "Improve LocalTransfer UX"?

Please make a ticket @Aditya-Sood

@macgills
Copy link
Contributor

@Aditya-Sood it looks like you have run afould of travis being an annoying inconsistent environment.
I will make a PR to dev to fix it, then you will have to merge dev in again for a clean build

@mhutti1 mhutti1 self-requested a review August 14, 2019 14:59
@macgills
Copy link
Contributor

@Aditya-Sood merge dev in again, it should work now. Hopefully we get a green build

@Aditya-Sood
Copy link
Contributor Author

@mhutti1 are there any changes for this PR?

Copy link
Contributor

@mhutti1 mhutti1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good to me. Will leave to @macgills merge though as he has given the most feedback.

@macgills
Copy link
Contributor

All good to me

@macgills macgills merged commit 3769831 into develop Aug 15, 2019
@macgills macgills deleted the feature/LocalFileTransfer branch August 15, 2019 08:30
@Aditya-Sood
Copy link
Contributor Author

@julianharty I haven't knowingly changed any file permissions. I wasn't aware that the new pre-commit hook added the .idea/codestyle file to my repo (I figured something was wrong with my local repo's .gitignore, which is why I removed all files and re-added them).

Maybe re-adding is what caused the file permissions to change? If you could tell me what the permissions should be I'll more than happy to fix them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants