search

ltguillaume / droidshows

A Reboot of DroidSeries Offline TV Shows Tracker
https://codeberg.org/ltguillaume/droidshows
GNU General Public License v3.0
83 stars 20 forks source link

[FR] Wishlist, New Fork #108

Open warren-bank opened 2 years ago

warren-bank commented 2 years ago

I've only just recently found this app. I'm absolutely loving it. Great job!

Here are a few suggestions for improvements:

ltguillaume commented 2 years ago

Wow, that's a lot... Let's start with number

  1. (FLAG_ACTIVITY_NEW_TASK). Please have a look if this works: I've added it to every intent for IMDb, Wikipedia and custom external resources.

DroidShows_7.11.5_FLAG_ACTIVITY_NEW_TASK.zip

  1. "Sort by unseen" does not do what you think it does. It sort by amount of unseen aired episodes. So if you want to binge a lot, you'll see the shows in the order of available episodes. I won't change the behavior, but I'm open to a name change in order to make this clearer. I don't know what use it would be to have a "Sort by unseen (newest first)" feature t.b.h.
warren-bank commented 2 years ago
  1. works great..
    • one-line change that makes a huge UX improvement, imho
    • I love having the "external resources" feature to cross-reference each show with any number of URLs w/ one as default
    • I link each show to webpages that have VOD for episodes in the given series
    • without this flag, I have a hard time remembering which season/episode is next by the time the site loads (and userscripts run)
    • with this flag, it's easy to jump back and forth between DroidShows and the app used to browse the webpage URL
    • also..
      • since I was using an apk signed by F-droid, I needed to uninstall and do a fresh reinstall.. rather than an update.. because the author signatures are different
      • I was worried whether your backup/restore would lose any data.. because I've accumulated a long list of shows (mostly archived)
      • everything worked great.. so thanks for that :)
  2. regarding sort order.. I see that this code is how "sort by unseen" is processed
    • basically: order by count_of_unseen_episodes DESC, timestamp_of_release_of_next_unseen_episode ASC
    • imho, a chronological sort would be very useful:
      • shows with 1-or-more released episodes that have not yet been seen should be grouped first
      • order by timestamp_of_release_of_next_unseen_episode DESC
      • ie: order from newest release first.. to oldest release last
      • shows without any unwatched episodes should be grouped afterward
      • order by timestamp_of_release_of_next_unseen_episode ASC
      • ie: order by date that next episode will be released.. from soonest to farthest into the future
ltguillaume commented 2 years ago

@warren-bank 2. Sorry I fail to see te use in that. Why would you want to have a sorting order from newest release to oldest when new aired episodes are available? The intent of the current sorting order is to know for which shows it's safest to start binging without having to wait long for new episodes. What issue would your proposed sorting order solve?

warren-bank commented 2 years ago

In contrast to binge-watching, when a series airs a new episode periodically (lets say one per week).. it's nice to have a sort option to see newest episodes at the top.

ltguillaume commented 2 years ago

Hmz, I always use the Pin feature for that, together with Exclude seen. It's much easier that way: the shows you're actively following will pop up at the top as soon as there are new episodes available. They will show up on the day of the broadcast.

Ibuprophen commented 2 years ago

I understand what @warren-bank is referring to, but as an app that provides this type/way of tracking (show,, episode, etc...), I (personally) think this is just right.

It's simplicity (without the Bling, Banners, etc...) is awesome for its functional role.

This isn't to mean that there isn't room for improvement (as any app isn't perfect in on itself). I just believe that some major/minor features could (potentially) over complicate it's overall functionality as it was meant to.

This is just a personal opinion, suggestion, etc and I hope I had expressed it okay via text. πŸ‘

~Ibuprophen

ltguillaume commented 2 years ago

Well, I'm still willing to look into small improvements. Bigger ones are just not really in the cards, because frankly, it's sort of a miracle that DroidShows still works, it using the APIv1 of TheTVDB and all...

Ibuprophen commented 2 years ago

I understand @ltGuillaume. πŸ’―

I did notice this too and I think that I read that they were continuing the backwards compatibility for the API based upon various feedback from the issues experienced by others with the newer API.

I just figured that once something big goes wrong, it'll probably/might be something tied to the API... LOL!

~Ibuprophen

ltguillaume commented 2 years ago

@warren-bank Just wanted to say that I'm following your fork's development and it's pretty great! πŸ˜ƒ

warren-bank commented 2 years ago

Thanks.. really appreciate it! I was going to say something once I finished the set of features that I'm actively working on.

In a nutshell.. right now I'm just ironing out some features that aren't related to the API. Once that's done, I'll start another branch to perform migration from thetvdb to tmdb. That should ensure long-term survival.

ltguillaume commented 2 years ago

Sounds great! I hope you find a way to reliably migrate the shows. Obviously the name only is not enough, but most shows have an IMDB id and a Zap2it id, that could help out. For example, in my database there are only two shows that don't have an IMDB id.

I also have a few fixes that I did only locally (shame on me), so I'll figure out what's worth implementing and let you know.

warren-bank commented 2 years ago

By any chance.. does one of your fixes include figuring out the reason that the app crashes when scrolling/flinging through the list and it reaches the last entry? That's a major annoyance.. and I was just about to look into it.

Without having started looking (ie: intentionally causing the crash and then inspecting logcat).. I'm guessing it's a memory issue that would be solved by using a RecyclerView (if we were using support libs.. which I totally agree would add some unwanted bloat). But who knows.. since each list item is so lightweight.. maybe it's something else..

update: I'm not sure what to make of this.. I'm testing the release thetvdb/007.11.05-09API.. and can no-longer reproduce this kind of crash. I have no idea if it was inadvertently fixed (in my forked repo) somewhere along the way. Profit?!

ltguillaume commented 2 years ago

Do you have this on multiple devices? Because I've never experienced this. Perhaps reinstalling and restoring the database with my latest build also fixes it on your device?

warren-bank commented 2 years ago

The Android TV Box that's attached to my bedroom TV.. currently using.. is still running your build; the one you uploaded to this issue w/ the added "FLAG_ACTIVITY_NEW_TASK" flag when starting Intents. The box is old.. MXIII running Android 4.4 w/ 2GB RAM. Still works great.. so I haven't bothered to replace it.

Anyway.. I just ran a quick test on it (while a video player continued to stream video in the background).. and the crash happened.. and logcat shows (abridged):

where the originating code is here

just to be on the safe side.. I think I'll add a try/catch to prevent this type of crash on low-memory devices.

update: here is the relevant commit, which is included in release thetvdb/007.11.06-09API

update: OutOfMemoryError is a fatal exception.. catching it doesn't prevent a crash.

update: commits 1 and 2 work to conserve memory by resampling every thumbnail and only caching/displaying bitmaps that are 100px x 100px. I don't know how long a list needs to be for this to consume enough heap to lead to a crash.. but my list is 140 long, I'm using a fairly ancient device, and it's working great.. no lag at all.. and more-importantly.. no crashing!

room for improvement: my methodology for cropping the posters is open for discussion. The posters appear much taller than they are wide. After resampling, I'm taking the full width and cropping height (to make a square) equally from top and bottom to take from the center. It looks fine to me, but there is definite crop loss.

Ibuprophen commented 2 years ago

I had done a fresh install test on both versions (individually) and, thus far, saw a minor issue between the two.

The following is a Screenshot of the latest version from @ltGuillaume (version 7.11.5) which shows the artwork/cover images as it should be.

Screenshot_ver 7 11 5

The following is a Screenshot of the latest version from @warren-bank (version 007.11.07-09API) and (as you can see) had noticed the artwork/cover images not sized correctly.

Screenshot_ver 007 11 07-09API

Other than the artwork, I didn't notice (personally) anything that jumped out at me, but I would definitely let you know. :-))

I've gotta state that I'm also very impressed with the contribution that @warren-bank had done too.

Great Job!!!

~Ibuprophen

ltguillaume commented 2 years ago

  1. I added the 3 commits I only had locally (one of them is adding FLAG_ACTIVITY_NEW_TASK), so you can cherry-pick if you need them in your fork, too.

  2. I never furthered the implementation of the optional DVD order. If you think it'd be a useful feature, too, would you like to have a look at it? Otherwise, I probably will, but not in the near future.

About the OutOfMemoryError, that's so odd, really. I've been using DroidShows with lists of at least 80 shows on an HTC Desire (2010) until 2020 (yes, it's been my daily driver for 10 years πŸ˜›). I thought that would be a very good benchmark for memory issues in any case. I did some testing and found the current way to be very responsive and not too hungry. Android does moest of the management in lists anyway.

That being said, and thinking about this some more, the Desire doesn't have a high resolution (800x480). I don't even remember anymore how the image sizes are calculated before saving them, but it's possible that the image sizes explode on high-res displays.

Either way, if the issue @Ibuprophen explained is to be fixed and considering high-res displays, I guess there's something to say for using a different approach for showing the posters in the ListView.

ltguillaume commented 2 years ago

Also, I probably see the effect of https://github.com/warren-bank/fork-Android-DroidShows/commit/891a1ba9eddee5eb64e970bdb591392f20e6de39 on @Ibuprophen's screenshot https://user-images.githubusercontent.com/18060271/170116416-5204ca7a-fa05-4b75-9d6a-7a331cd9527b.jpg and I'd say that's a definite regression, too. What was the idea exactly?

warren-bank commented 2 years ago

I'll re-address the icon cropping. I'm currently in the weeds with respect to data API migration. But before committing any code for that, I'll change the icon methodology: only resize such that width is always 100dp.. allow height to maintain aspect ratio.. update layout as such. Shouldn't be a problem... image size will still be very small.. aspect ratio will be maintained.. no cropping will occur. TBA..

PS: the methodology in the old code was to: resize by height (25% of screen) and maintain aspect ratio for width. Using your phone's specs as an example: screen height is 800px.. height of images after resize are 200px. Larger (or higher density) screens produce larger images that are accumulated in memory. With regard to memory, not horrible.. but we can do better. With regard for layout, you were lucky that thetvdb must pre-process all of its posters to a normalized aspect ratio; otherwise the icon widths could've been wildly inconsistent.. and the list would've looked a complete mess.

update: it just occurred to me.. since my Android box is connected to my TV over HDMI.. it's probably seeing that my screen dimensions are 4K.. 3840px width x 2160px height.. so if "icons" are resized to 25% of this height.. that's 540px each.. ouch! That could probably lead to a memory-related crash with only a moderately sized list.

ltguillaume commented 2 years ago

With regard for layout, you were lucky that thetvdb must pre-process all of its posters to a normalized aspect ratio; otherwise the icon widths could've been wildly inconsistent.. and the list would've looked a complete mess.

That's true. but I knew about this, so I could make use of it πŸ˜‰

warren-bank commented 2 years ago

release: 'thetvdb/007.11.09-09API'

ltguillaume commented 2 years ago

Impressive! I think maybe we should look into how you could co-manage this very repo, so releases to F-Droid could start up again, while users can seemlessly update? What do you think?

I could build & sign the APK's here, but the signing key for DroidShows is used only for DroidShows, so I can eventually share it with you if you'd like.

warren-bank commented 2 years ago

10 days later.. and version tmdb/008.00.00-09API is born.

I could use some testers.. because about 85% to 90% of the code in the entire app has been either replaced or rewritten.

notes:

Ibuprophen commented 2 years ago

This is great news @warren-bank!

I'll test it out and report back anything that I personally notice.

Is there anything specific on your mind that you would like me to keep an eye on or just in general?

~Ibuprophen

warren-bank commented 2 years ago

nope.. just anything that's not working right, or could be improved upon.

As for speed.. there's nothing I can do. This API requires many more requests to accumulate the same quantity of information. Plus, it uses a library to convert JSON to Object models.. rather than extracting fields directly from raw XML.

In any case, now we have an option..

warren-bank commented 2 years ago

@ltGuillaume ..quick question

I just noticed your commit..

ltguillaume commented 2 years ago

@ltGuillaume ..quick question

I just noticed your commit..

* where the underlying issue is that when the name of a series is passed as a querystring parameter in a URL.. it should be encoded.. but hadn't been

  * in Javascipt.. we have: `encodeURIComponent(name)`
  * in Java.. we have: `URLEncoder.encode(name, "UTF-8")`

I ran into new problems with .encode, but I agree this is a hacky solution (as said in the commit message).

* when looking at the code where this occurs.. I also notice that:

  * `name = name.replaceAll(" \\(....\\)", "")`
  * what is the intended purpose for this?

    * are the 4 characters inside the parenthesis assumed to be a year? (ex: " (2022)")
  * looking at a random sampling of series names returned in search results.. none include a matching substring
  * and considering that the TMDB branch is using names from an entirely different API.. I'm very tempted to get rid of these regex search/replace operations.. but figured I'd ask first

This is because (at least in my collection) many show names have their year appended (e.g. Archer (2009)). I found that IMDB did not appreciate it if (2009) was included in the search term, so I decided to strip it from the show name.

As you said, TMDB might not append the year to show names.

ltguillaume commented 2 years ago

10 days later.. and version tmdb/008.00.00-09API is born.

I could use some testers.. because about 85% to 90% of the code in the entire app has been either replaced or rewritten.

notes:

* the API is now using TMDB v3 (rather than TheTVDB v1)

  * which is entirely different
  * TheTVDB v1

    * a request for a TV series returns a huge monolithic response that contains _all_ related data
  * TMDB v3

    * a request for a TV series returns only data about the series and a minimal amount of data about the number and size of its seasons
    * a request for a season returns a limited amount of data for the episodes in the season

      * this data isn't sufficient for our needs.. so this API endpoint isn't used
    * a request for an episode returns only data about the individual episode

* databases can be updated from older releases, which involves:

  * migrating to an entirely new database schema
  * re-adding all TV series

    * which can take a while
    * a progress dialog shows.. progress
  * updating the new DB with data from the old (ex: archived, pinned, external resources, seen, ...)

    * data about which TV series are "pinned" has been moved from shared preferences into the DB

* updates to each TV series involves:

  * updating data about the series
  * replacing data about the seasons (which is included in the response for data about a series)
  * requesting only the new episodes

    * old episodes are left intact, and no API requests are made to modify them

Wow! Did I miss a few commits somehow, or is basically everything you describe here in https://github.com/warren-bank/fork-Android-DroidShows/commit/e237faeba00e42605cf33538d9c5ff206e997e36 and https://github.com/warren-bank/fork-Android-DroidShows/commit/8cb1a56463b2db77464d1f0c0c25c26aae15f9e4 ?

Either way, impressive work! I'll be sure to take a better look at all the changes soon, just wanted to thank you first πŸ˜ƒ

warren-bank commented 2 years ago

Nope. You didn't miss anything; I was working offline. Maybe it's just me, but I don't like to commit code in a state that can't compile. And to do this.. I needed to tear everything apart and break it.. in order to put it back together again.. differently. Once I had a version that compiled and was ready to show.. then I commited the end result. Now, as I make minor tweaks and improvements.. I'll make small individual commits.

Thanks for confirming that the regex was to strip off a year. Offhand, do you remember why URLEncoder.encode didn't work when you tested it? Encoding querystring values seems to be its raison d'Γͺtre. I could include a custom function that percent encodes any character that isn't [a-zA-Z0-9].. but it would be easier to use what's already provided.

ltguillaume commented 2 years ago

Nope. You didn't miss anything; I was working offline. Maybe it's just me, but I don't like to commit code in a state that can't compile. And to do this.. I needed to tear everything apart and break it.. in order to put it back together again.. differently. Once I had a version that compiled and was ready to show.. then I commited the end result. Now, as I make minor tweaks and improvements.. I'll make small individual commits.

Makes sense!

Thanks for confirming that the regex was to strip off a year. Offhand, do you remember why URLEncoder.encode didn't work when you tested it? Encoding querystring values seems to be its raison d'Γͺtre. I could include a custom function that percent encodes any character that isn't [a-zA-Z0-9].. but it would be easier to use what's already provided.

I just replaced every %26 .replaceAll() with Uri.encode() for the parameter strings and that works fine now. So I might have just screwed up before. I can't really remember, as I was feverish when I made the change, but I just wanted to help out @Ibuprophen as quick as possible.

Ibuprophen commented 2 years ago

@warren-bank, I did notice a little something...

I had first (of course) backed up the DroidShows folder to another location to keep a copy of my shows database.

I had decided to test your app as a fresh install where I had cleaned the cache, uninstalled the app and removed the DroidShows folder and such.

It installed fine, I added 1 show (as I did notice the slight slowness) and then when I selected to backup, I did quickly notice that it didn't create the backup directory as reflected in an error that it couldn't do so because it couldn't locate the directory.

I'll work with it some more and let you know of anything else.

Thanks a Bunch! πŸ‘

~Ibuprophen

ltguillaume commented 2 years ago

@warren-bank, I did notice a little something...

I had first (of course) backed up the DroidShows folder to another location to keep a copy of my shows database.

I had decided to test your app as a fresh install where I had cleaned the cache, uninstalled the app and removed the DroidShows folder and such.

It installed fine, I added 1 show (as I did notice the slight slowness) and then when I selected to backup, I did quickly notice that it didn't create the backup directory as reflected in an error that it couldn't do so because it couldn't locate the directory.

I'll work with it some more and let you know of anything else.

Thanks a Bunch! πŸ‘

~Ibuprophen

This might actually be something related to DroidShows in general and Android's increasingly restrictive storage policies? Are you sure you're not getting the same thing with a clean install of one of my latest releases?

warren-bank commented 2 years ago

That is odd.. this is all done pretty much identical to the way it was done upstream..

warren-bank commented 2 years ago

it would be helpful when odd behavior is described.. to also include the version of Android in which the behavior is observed.

as Guillaume said, Android consistently breaks user space with each update.

warren-bank commented 2 years ago

I see the problem (that causes an error when performing a backup on newer devices)..

on Android 6.0+:

Ibuprophen commented 2 years ago

@warren-bank & @ltGuillaume, that was a great suggestion that prompted me to take a look and found that it requires the user to enable the Storage Permissions in the System Settings.

I enabled it and it was all set. πŸ‘

Apps that requires certain permissions are typically set up to ask for those permissions via a pop-up screen (I hope I explained that okay... LOL!). This was why it didn't occur to off hand.

~Ibuprophen

ltguillaume commented 2 years ago

@warren-bank & @ltGuillaume, that was a great suggestion that prompted me to take a look and found that it requires the user to enable the Storage Permissions in the System Settings.

I enabled it and it was all set. πŸ‘

Apps that requires certain permissions are typically set up to ask for those permissions via a pop-up screen (I hope I explained that okay... LOL!). This was why it didn't occur to off hand.

~Ibuprophen

Great! FYI, @warren-bank has addressed this in https://github.com/warren-bank/fork-Android-DroidShows/commit/99e36efa77a3eecb522ba233038bb637400344f2

Ibuprophen commented 2 years ago

@warren-bank, I just tested a fresh install of DroidShows-008.00.02-09API-release and the permissions pop-up worked great.

But, restoring the database from the one by @ltGuillaume didn't work out well (for me anyways).

1 - The following image reflects the file backup placed in the backup directory (note the file size).

1 - Copied Backup

2 - The following reflects 1 (of 2) toast messages.

2 - Restore Toast 1

3 - The following reflects 2 (of 2) toast messages and you'll see nothing restored.

3 - Restore Toast 2

4 - The following reflects the results of the database files in the backup directory after performing the restoration (please note the file sizes).

4 - Restore Results

5 - Just as a reference, the following is what's reflected in the latest version released by @ltGuillaume.

5 - DroidShows by ltGuillaume

... on another note...

The following reflects the huge add symbol after searching for a show, but that's minor in on itself.

6 - Large Add Symbol on Right

I hope I had explained the above okay via text.

Thanks a Bunch!

~Ibuprophen

warren-bank commented 2 years ago

did the DB migration open a progress dialog.. for a long/slow update?

if you want to share a DB backup file.. I can test it here. the one I'm testing is small, but migration works well.

Ibuprophen commented 2 years ago

No. I waited a few minutes and the progress indicator didn't pop-up.

Afterwards, I had performed a fresh install of the latest version by @ltGuillaume to perform a restore to be sure that the backup was a good one and it imported without an issue.

The following is a copy of my backup database for you to try.

DroidShows.zip

I'm not in a rush/race. Please take your time as I do understand that it can be challenging in application developments (as I have a few of my own). πŸ‘

~Ibuprophen

ltguillaume commented 2 years ago

@warren-bank Perhaps it's an idea to create the releases with a different package name (e.g. bank.warren.droidshows) and app name (DroidShows Ξ²?) for as longa as your fork is in alpha/beta? Could be useful for testing purposes.

Ibuprophen commented 2 years ago

I agree with you @ltGuillaume regarding the temporary change in the package name during the Alpha/Beta testing.

Also a temporary app name as well so others will be able to identify which DroidShows app is which (for example "DroidShows 2" or something).

~Ibuprophen

warren-bank commented 2 years ago

@Ibuprophen

@ltGuillaume

Ibuprophen commented 2 years ago

@warren-bank, what I meant by the size was for identifying it for comparison.

I deleted the initial (blank) database, placed a copy of my backup database.

Then after the restore was performed (as I had reported), the backup directory looked like the app had taken my copy and renamed the extension to "db1" and created an empty one with the "db" extension.

~Ibuprophen

warren-bank commented 2 years ago

ok, well.. idk.. but what I can say.. is that the update process began after I restored your DB file.. and it's currently running; I'll upload the resulting .db file after it completes.

but.. I have no idea why it didn't start for you.

PS: testing on Android 8.1

warren-bank commented 2 years ago

considering how long this process can take.. I'm thinking that I should add a wakelock.. for cpu and wifi, then release it after the migration finishes.

warren-bank commented 2 years ago

yep.. sure enough i turned my screen off.. and the phone went to a low power state that killed the update.. resulting in an incomplete migration that only has the shows that were updated before the process was killed.

that being said.. the process was working; I have a few Star Trek shows in my list now; but I definitely need a wakelock.

I hope that's sufficient.. and that I won't need to use a foreground service.

ltguillaume commented 2 years ago

Yeah, I didn't necessarily mean permanently, I just saw @Ibuprophen going back and forth between the two by uninstalling, and I would like to test it myself, ideally without touching the current install until it's stable. It'd also be nice to have both installed at the same time and then be able to compare the info of TMDB and TVDB.

ltguillaume commented 2 years ago

As for the process of restoring/updating, I found that on my devices DroidShows continued either while the screen was turned off or at least after the screen was turned back on again, but I was always on a custom ROM, so hard to say how all those stock ROMs will react to it.

warren-bank commented 2 years ago

status update..

the following are all done:

the result:

warren-bank commented 2 years ago

status update: