Skip to content

Conversation

@wmergenthal
Copy link

@wmergenthal wmergenthal commented Jun 26, 2025

I have not observed a failure with Reader sending a book request to Desktop, but the mechanism that does this extends a class (AsyncTask) that is deprecated as of API level 30. This changeset offers an alternative implementation. It's mostly the same stuff - UDP socket and JSON book request - but is just lifted out of the extended AsyncTask class. I could not see the need for the extra layer and extra thread, as the main thread does not need to resume listening for adverts after making a book request.

I left in some debug output code to log the content of the book advert received from Desktop. It is commented out. I have found this very useful, but it should probably be deleted in production code. Perhaps the same will be true of the handful of other lines, still uncommented, showing the progress of an advert's reception and processing. But I would encourage that they be retained if possible.


This change is Reviewable

This change is not motivated by a failure I or anyone else has observed. But using deprecated code is not good long term, so this change replaces AsyncTask (deprecated in API level 30) with a Socket. This also has the minor benefit of reducing the thread count by 1.
Some debug output is still present. I will remove it before making a PR.
Remove many debugs, modify others. Works on wired connection at home, next is to test on Wi-Fi at JAARS.
Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Looks good overall.
I think I'll have @JohnThomson take a look because

  1. Though I'm sure it has faded far into distant memory, I think he probably wrote this originally and might have an idea why we make it asynchronous in the first place.
  2. He has a better head for threading than me.

Remove comment referencing 'SendMessage', a class being removed.
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.

2 participants