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

[FR] Wishlist, New Fork #108

Open
warren-bank opened this issue Mar 21, 2022 · 144 comments
Open

[FR] Wishlist, New Fork #108

warren-bank opened this issue Mar 21, 2022 · 144 comments

Comments

@warren-bank
Copy link

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

Here are a few suggestions for improvements:

  • add "FLAG_ACTIVITY_NEW_TASK" flag to Intent
    when launching an "external resource" URL,
    so DroidShows remains visible in the recents menu
    as the top-most Activity in the history stack for its task
  • add menu item to main menu:
    • "Sort by.."
      opens a dialog with options:
      • Sort by name
      • Sort by unseen (oldest first)
        • note: this ascending order is currently available
      • Sort by unseen (newest first)
        • note: this descending order would be very useful
  • add menu item to season context menu:
    • "Seasons after here are unseen"
      (because I sometimes like to rewatch a series from the beginning)
  • add option to configure the background color
    • default:
      • System Wallpaper
    • solid color:
      • White
      • Black
  • add option to disable the pull-to-update feature
    (because I often trigger it by accident)
  • add option to manage labels (ex: similar to Gmail) to categorize shows
    • default:
      • Active
      • Archive
    • use case: multi-user
      • John
      • Jane
    • use case: genre
      • Reality TV
      • British detectives
    • notes:
      • this would require additional UI to manage label associations per show
      • this would effect "update" behavior
        • currently, only "Active" shows are update (ie: not "Archive")
        • the menu item "Update shows" (ie: pull-to-update behavior)
          should only update the shows associated with the currently selected label
        • a new menu item "Update all" could be added
          that would trigger an update to shows in all labels
@ltguillaume
Copy link
Owner

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
Copy link
Author

warren-bank commented Apr 17, 2022

  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
Copy link
Owner

ltguillaume commented Apr 17, 2022

@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
Copy link
Author

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
Copy link
Owner

ltguillaume commented Apr 21, 2022

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
Copy link

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
Copy link
Owner

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
Copy link

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
Copy link
Owner

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

@warren-bank
Copy link
Author

warren-bank commented May 22, 2022

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
Copy link
Owner

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
Copy link
Author

warren-bank commented May 23, 2022

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
Copy link
Owner

ltguillaume commented May 23, 2022

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
Copy link
Author

warren-bank commented May 23, 2022

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):

  • OutOfMemoryError at:
    • android.graphics.BitmapFactory.decodeFile()
    • android.graphics.drawable.Drawable.createFromPath()
    • nl.asymmetrics.droidshows.DroidShows$SeriesAdapter.getView (line 1897)

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
Copy link

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 added a commit that referenced this issue May 25, 2022
so DroidShows remains visible in the recents menu
when launching an "external resource" URL
@ltguillaume
Copy link
Owner

ltguillaume commented May 25, 2022

  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
Copy link
Owner

Also, I probably see the effect of warren-bank@891a1ba 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
Copy link
Author

warren-bank commented May 25, 2022

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
Copy link
Owner

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
Copy link
Author

release: 'thetvdb/007.11.09-09API'

  • fixes the icon issue
  • fixes Sort by date of next episode #106 by adding the requested sort option
  • fixes the fact that the episode details page was able to add calendar events for the air date of episodes that had already been aired
    • now that feature is only enabled for episodes that have not yet been aired (ie: the date is at some time in the future)

@ltguillaume
Copy link
Owner

ltguillaume commented May 26, 2022

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.

@ltguillaume ltguillaume changed the title [feature request] wishlist.. [FR] Wishlist, New Fork May 26, 2022
@warren-bank
Copy link
Author

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

@Ibuprophen
Copy link

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
Copy link
Author

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..

  • the old version of the app will continue to work great.. so long as TheTVDB v1 remains alive
  • the new version (ie: v8.0.0+) is available.. and should continue to work for a long time to come

@warren-bank
Copy link
Author

@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")
  • 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

@ltguillaume
Copy link
Owner

@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
Copy link
Owner

ltguillaume commented Jun 13, 2022

I agree with @Ibuprophen. I understand the appeal to have the covers a bit larger than originally in DroidShows, simply because they're more helpful to recognize the show if they're a bit bigger, but I feel they're way too big right now.

if the title was elevated to a full-width row above the image, then the overall height of each item in the list would increase.

Could still work if the cover was smaller, but I don't think it'd be necessary if the cover would be resized to somewhere between the current and the old size.

however, I'm thinking that allowing the title to wrap to additional line(s) wouldn't be such a bad idea. Any thoughts?

I'm afraid that'll look really bad.

@warren-bank
Copy link
Author

warren-bank commented Jun 13, 2022

I'm not married to the current sizing logic.. but to summarize exactly what it is:

  • in the ListView:
    • width of image is the smaller of:
      • 100 dp
      • 20% of the screen's width in portrait mode
    • height is determined by width to maintain the original image aspect ratio
  • in Series details:
    • width of image is the smaller of:
      • 200 dp
      • 33% of the screen's width in portrait mode
    • height is determined by width to maintain the original image aspect ratio

this discussion seems to imply that we could decrease the height without affecting the width..
which would require cropping..
which (based on my earlier testing) looks really bad.

regarding truncating the names of series..
I'm much more prone to show the entire name..
as this is vital information to a user;
which (imho) is more important than a question of aesthetics.

to be perfectly honest..
I never noticed the truncating of series names in my own usage..
because I always use this app on a large TV screen.. on which it's a non-issue.
But on a small-screen device, I would expect to see all of the important details.

@Ibuprophen
Copy link

Just as @ltguillaume had mentioned, I do like having the image solely for a quick recognition of the listed series,. Resizing them down would be a good thing to me, but not too small... LOL!

I'm more of a Simplistic & Functional person for certain apps like this.

Adding a 2nd line would be a good idea, but wouldn't it be better for the app to recognize that the title required it so those series that doesn't need it would remain using the single line?

Also, if the title text would be resized, maybe changing the color would allow it to stand out a bit more (not referring to something like yellow that would just abuse the eyes a bit... LOL!).

1 more thing regarding the image, I'm only referring to the list itself. The image in the series details is fine as the full title is already good to go.

I hope I had explained this okay via text.

~Ibuprophen

@warren-bank
Copy link
Author

well.. I can only speak to my own personal preferences.. but I rather like the sizing of the images as it is now; it looks good on my smallest handset.. and it looks good on my largest TV.

regarding the text.. I just released a minor update that allows the title to span up to 3x lines (but only if necessary). I'm not eager to change: font, color, size, etc.. I rather like it how it is. (ie: keeping it simple and clean)

@warren-bank
Copy link
Author

warren-bank commented Jun 14, 2022

i honestly didn't think anybody would feel so strongly about image size..

what I could do..
is to move the max width (as a percentage of screen width) to a user preference..
which would default to 20%

however, my concern is what to do when a user changes this value?
if the value is decreased, then the app could resize all of the current images (on disk).
but.. if the value is increased, then??
..do we delete all of the current images (on disk).. and redownload/resize a new image for every series?

that seems like a whole lot of work (for the app to perform).. for very little reward.
idk..

although..
come to think of it..
the width that a user can choose for the listview image could be constrained..
such that it cannot be larger than the image used on the series description (ie: 200dp / 33%)..
which would allow the listview image to always be regenerated by resizing the larger/series image..
which removes the need to redownload..
so if the allowed range were.. lets say.. 10% to 33%
which would produce a resized image having a width that is the smaller of: 100dp or (XX%)(screen width in portrait)
..I think that could work

@ltguillaume
Copy link
Owner

ltguillaume commented Jun 14, 2022

I think it would be a good idea to look into just using a single cache for the images, without having to recreate all the thumbnail images:

  1. I believe the "medium size" images already have bigger dimensions than necessary, even for the show details activity
  2. You could just resize them to that maximum size in the show details and give the user the option to scale down from there for the shows list, by only changing the dimensions in the getView(), not by having another set of thumbnail files. Pretty sure it's gonna be enough for your out-of-memory issue to do it like that, too.

@warren-bank
Copy link
Author

personally, I don't like the idea of dynamically resizing images in a list view..
where each row that gets loaded requires all of that extra work;
I like the current methodology much better..
which is to know exactly what size(s) each image will be displayed at..
resize them once (per size).. and load them (as needed) without any dynamic resizing or scaling.

sure.. it requires a little extra disk space on the frontend..
realistically, very little..
and it saves a ton of cpu load and memory on the backend.

to be perfectly honest..
I really kinda hate the idea of allowing the user to change the image size..
if it becomes an issue that gets enough interest, then I'll add it..
but for now.. I'm going to hold off

@Ibuprophen
Copy link

@warren-bank, I had just updated to the "TV-Tracker-008.00.10-09API-release" and its much better.

The size of the images are fine for myself and I'm very happy that the longer full series titles are viewable (based upon my list of shows). Thanks a Bunch!

I did notice something else that applies to both from @warren-bank & @ltguillaume.

When an alternative backup directory has been established successfully, it (apparently) only changes when manually backing up.

When you select the "Automatically perform daily backups" opinion, that still utilizes the default directory.

I hope I had explained this okay via text.

~Ibuprophen

@ltguillaume
Copy link
Owner

ltguillaume commented Jun 14, 2022

@warren-bank, I had just updated to the "TV-Tracker-008.00.10-09API-release" and its much better.

The size of the images are fine for myself and I'm very happy that the longer full series titles are viewable (based upon my list of shows). Thanks a Bunch!

I did notice something else that applies to both from @warren-bank & @ltguillaume.

When an alternative backup directory has been established successfully, it (apparently) only changes when manually backing up.

When you select the "Automatically perform daily backups" opinion, that still utilizes the default directory.

I hope I had explained this okay via text.

~Ibuprophen

I can't reproduce this with my fork.

@warren-bank
Copy link
Author

warren-bank commented Jun 14, 2022

I can't reproduce it either:

  • enabled auto backups
  • took a manual backup and chose a new non-standard directory
  • after backup, was prompted which directory to make default
    • selected the new non-standard directory
  • exited app
    • triggering an automatic backup
  • a new database file produced by the automatic backup (can tell by filename that doesn't contain a timestamp) was written to the newly selected non-standard directory
    • my default directory was and is still clean
  • deleted the database files
    • in fact, deleted the new non-standard directory and all of its contents
  • restarted app
  • exited app
  • new non-standard directory was recreated, and a new automatic backup database was written to it

@ltguillaume
Copy link
Owner

@warren-bank Just so you know, HH:mm seen was an explicit request by a user. I think the amount of episodes seen is also useful, but it could just as easily be combined into something like Episodes seen: xx (about XhXXm).

@warren-bank
Copy link
Author

v8.0.12 adds a new option that allows the images in the ListView to be dynamically resized.

the approach is kind of a "best of both worlds" solution..
by default, images are resized upfront and presented without any additional scaling.
using this option, images can be dynamically resized at runtime (by configuring attributes of the ImageView).
so.. it's up to the user to choose which resizing strategy they would prefer to use.

@Ibuprophen
Copy link

@warren-bank, I just wanted to let you know that everything has been working well as of today's release.

Though I can't state this for every single feature since I don't typically use all of them myself.

Thanks a Bunch! 👍

~Ibuprophen

@ltguillaume
Copy link
Owner

ltguillaume commented Jun 17, 2022

@warren-bank I was thinking about #108 (comment) and your remark

  • if the season finale of a series airs tonight, then:
    • next unaired episode = 6 months from today

which is not true for my fork, so I thought maybe you've changed the behavior there. But judging from https://github.com/warren-bank/Android-Tiny-Television-Time-Tracker/blob/dc5614636a6f374f903f62936399c920ad6d5767/android-studio-project/TV-Tracker/src/main/java/com/github/warren_bank/tiny_television_time_tracker/database/DbGateway.java#L944 it's still the same.

@Ibuprophen
Copy link

@warren-bank, I had just updated the app to "TV-Tracker-008.00.16-09API-release" and, when I performed an update to the shows, it was updating slowly and then crashed halfway through.

Here's a Screenshot and a Crash Report (hoping they're helpful).

Crash Screenshot - TV-Tracker-008 00 16-09API-release

Crash Report - TV-Tracker-008.00.16-09API-release.txt

The prior version gave me no issues.

Thanks a Bunch!

~Ibuprophen

@warren-bank
Copy link
Author

warren-bank commented Jun 17, 2022

interesting..
my guess is that this API method returns null instead of a (potentially empty) List

suppose I should wrap the loop within a test to check that the List is non-null..

I'm curious which episode in which series raised this error?..

I'm currently re-migrating my entire 140-series database..
about half-way through right now..
but haven't encountered any errors.

update:

  • just rechecked the progress.. finished without any errors
  • for my own curiousity.. should your previously migrated DB file reproduce your error.. if I were to restore and update?
    • confirmed.. it does crash with the same error
    • fix coming ASAP..

@ltguillaume
Copy link
Owner

ltguillaume commented Jun 17, 2022

@warren-bank Apparently there's a lot more going on in terms of database transactions with the TMDB implementation. I think you could vastly improve the performance by

  1. not disabling WAL (db.disableWriteAheadLogging())
    This does mean, however, that you'll have to start creating checkpoints before backing up (PRAGMA wal_checkpoint(full)) or use SQLite's backup API (https://www.sqlite.org/backup.html) in order to create backups (not sure how to implement that).
  2. (probably) using more db.beginTransaction();/db.setTransactionSuccessful();/db.endTransaction(); to write to the database in batches.

@warren-bank
Copy link
Author

@Ibuprophen
new version fixes the problem..
thanks for letting me know.

@Ibuprophen
Copy link

Just updated it and no issues experienced @warren-bank.

Thanks a Bunch! 💯

~Ibuprophen

@warren-bank
Copy link
Author

warren-bank commented Jun 18, 2022

@ltguillaume

references:

observations:

  • this code doesn't actually do anything..
    • enableWriteAheadLogging is never called
    • disableWriteAheadLogging turns off features enabled by enableWriteAheadLogging ..which are disabled by default
  • regarding enableWriteAheadLogging..
    • the features it enables pertain to being able to write to the database at the same time from multiple threads
  • in any case..
    • I wouldn't want to enableWriteAheadLogging ..so there's no harm in leaving it as is

@ltguillaume
Copy link
Owner

ltguillaume commented Jun 18, 2022

@warren-bank I know that. I added disableWriteAheadLogging() because the amount of db operations wasn't that big and performance was okay. But there's much to gain with this new implementation.

I know for sure that this was needed, but some Android versions had WAL on by default, some didn't, AFAIK. So I needed to add this when I switched to a newer Android version in order to quickly fix backups.

AFAIK it's supposed to be faster because it doesn't have to write changes to the main database every time, but logs it (in a separate small file) and then commits it in batches to the main database file.

@warren-bank
Copy link
Author

warren-bank commented Jun 18, 2022

well..

currently.. during migration:

  • all work is performed on a single thread (..separate from the UI thread)

in theory it would be possible to rewrite the migration code to:

  • re-add series using a new thread per series
  • allow each thread to write to the DB in parallel
  • detect when all threads have completed

then.. resuming sequential processing

however, personally.. I have no interest in implementing parallelization;
I just don't see migration as that important..
it works pretty well as it is, and that's good enough for me.

@ltguillaume
Copy link
Owner

ltguillaume commented Jun 18, 2022

Do you have evidence that the fact that the code isn't multithread is the actual bottleneck, instead of the (amount of) database queries? Or more specifically, the writing-to-the-database because of the amount of separate queries?

@warren-bank
Copy link
Author

warren-bank commented Jun 18, 2022

the time to execute each DB update.. nanoseconds
the time to download and parse 100+ API requests w/ JSON response payloads per series.. minutes

for each series:

  • API request for series
    • write to DB: series, seasons, genres, actors
    • API request for each season in series
      • API request for each episode in each season in each series
        • write to DB: episodes, writers, directors, guest stars

so:

  • the number of API requests per series is a function of the total number of episodes

I don't really think that evidence is needed..
but feel free to run some benchmarks to compare the relative amount of time spent performing download/parse vs. DB insert

@warren-bank
Copy link
Author

totally off-topic.. but it's been a productive week; I wrote another app that you two might find useful.. and just released an initial version. a few additions/enhancements are in mind for near-term updates.

@Charles7z
Copy link

Just to add a little info.

I just installed DroidShows and Tiny... I started with DroidShows, loaded all my TV shows, about 60, etc. Then discovered the fork, installed it, restored from DroidShows backup — Tiny... then downloaded the shows, it was very slow the first time, I've refreshed a couple times with each app and Tiny takes about 3x longer, still under a minute.

Note that I'm on rather slow data so it's noticeable.

Just thought you might want to know. And thanks for continuing this app, amazing I just found it 🤦‍♂️

@Ibuprophen
Copy link

@Charles7z, Upon the 1st restore of the Droid Shows database, it will take quite some time to accomplish as mine took around an hour with a little over a dozen shows.

Once this first restore is done, everything should work out much faster.

This is only because the TV Tracker App uses a more up-to-date API as well as reworked coding.

So the initial import is only converting the Droid Shows database information using the current API that will always result in this slow import.

It's hard to explain via text, but maybe @ltguillaume and/or @warren-bank will be able to provide a better explanation.

Please Enjoy! 👍

~Ibuprophen

@ltguillaume
Copy link
Owner

ltguillaume commented Jun 30, 2022

@Ibuprophen it took an hour for a bit over 12 shows? That's crazy! 😉

@Charles7z
Copy link

I've tested a lot of move/TV tracking apps and none have every taken an hour to sync, and they are synching thousands..... Cathode which keeps a good amount of info for offline doesn't even take that long.

I'm thinking something was wrong with that particular sync.

On a side note....

Does anyone know of a similar app for movies? Something that's offline. Everything I've found required account login or doesn't keep data for offline viewing.

Thx again

@jmichael2497
Copy link

jmichael2497 commented May 15, 2023

just spotted and browsed across all this, looks like i encountered mostly already known issues, except it seems like restore of 5MB (90 shows) DroidShow backup from F-Droid 7.11.2 still not working for me (missing watched status) in LOS (A13) so opened issue.

A4T restore missing watched status

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

No branches or pull requests

5 participants