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

Complete Refactor/Rewrite #292

Closed
da3dsoul opened this issue Feb 6, 2019 · 80 comments
Closed

Complete Refactor/Rewrite #292

da3dsoul opened this issue Feb 6, 2019 · 80 comments
Assignees
Labels
Core A core change and a lot of work is needed Enhancement Improving on what's there Kodi Agnostic It does not involve any specific version of Kodi

Comments

@da3dsoul
Copy link
Contributor

da3dsoul commented Feb 6, 2019

Yes, I mean complete. I will do this for APIv3 most likely. The current code base is a steaming pile of garbage that runs fast only because I fix it any time anyone touches it. It needs to be thrown in the garbage where it belongs. Since most of this will need to be moved at the least for APIv3, I suggest doing it at the same time.

I'm a strong proponent of OOP. No BS Procedural Programming here, please. That's better kept to small scripts. I hate Python, so the best way to deal with it is to not be pythonic.

Included in this would be the proper separation of Nakamori.Service for scrobbling and handling auto-next episode, playlists, etc. We could possibly make that easier by hooking into the plugin available for that.

We will wrap a GuiBuilder object that functions as a list of items. It can take custom objects, such as groups or series, and convert them. At the start of the script, we will create a GuiBuilder, which starts the directory in Kodi, pass the object for adding things to, then when we are done, we call GuiBuilder.finalize() and it'll build the final UI. This should clean up of lot of our long classes of custom menus with tons of ifs. GuiBuilder will have a reference to the current listitem and API data for processing, so we don't need to pass around a dictionary or 4 with no obvious documentation.

We will also refactor out error handling and keep it globally as a set of Error objects with a priority, message, stacktrace, and optional function to call. The message is what is shown to the user. The stacktrace is logged, obviously. The priority determines which error message from the set is actually shown to the user. The highest priority will block operations and execute the provided function, such as going to the main menu, opening settings, or running the first time setup. We may expand these options later to possibly include uploading logs, contacting us, or linking to a FAQ. We may also add options to this to suppress logging when there's an obvious chain. For example, a network error will cascade and show a bunch of irrelevant errors.

Better debugging support is needed. We have what's needed, but I need to test it and see about an easier solution.

I won't try to enforce it, as I've never done it before, but we should have unit testing for all of our directory and API processing. I have no clue how that works in Python yet, so don't panic over it.

All kodi version specific processing should be done through override proxy objects. An example is here. This will allow us to do various things while changing very little and avoiding bloat when updating to new versions of Kodi. This will remove all of the if kodi_version < 18: crap.

We will rearrange the submodules/plugins to be more in line with the Kodi standards.

nakamori.plugin - will have only what is needed to provide a directory to kodi. No processing is done here. It is merely a thin wrapper.
nakamori.script - will have only what is needed to be called and forward functions from Kodi. No other processing is done here. It is merely a thin wrapper.
nakamori.service - this one I'm a bit torn on. There likely won't be much in this. It is only for handling playback events and whatnot, after all. For now, I say it's fine to keep processing in this.
nakamori.lib - This is where everything happens. I mean it. Basically everything else just calls something in here. When updating, this package will likely be the only one that changes unless there is a UI change in nakamori.script.

I will append this as I think about it more. I don't expect comments, but feel free if you have useful input.

@da3dsoul da3dsoul added Enhancement Improving on what's there Core A core change and a lot of work is needed Kodi Agnostic It does not involve any specific version of Kodi APIv3 Only Things that will not aim to be backwards compatible. It will target APIv3 and above only labels Feb 6, 2019
@da3dsoul da3dsoul self-assigned this Feb 6, 2019
@bigretromike
Copy link
Owner

You don't have to be rude. Glad to hear that you wish to continue your journey with Kodi.
If you want to remove the trash consider few:

  • python3 support
  • building custom gui aka calendar

If you target APIv3 then I don't need to worry about this for some time...

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 6, 2019

Oh I wasn't trying to be rude. It's not your fault explicitly either. Neither one of us had the experience in Kodi to write a decent looking plug-in, and I don't know what the original one that this was built on looked like, but now we do have the experience, so it's time for some cleanup

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 6, 2019

Yeah Python 3 may be the only target depending on how many users still use Krypton and below when APIv3 is ready

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 6, 2019

By making it more modular, custom menus like calendar should be easier

@bigretromike
Copy link
Owner

imo, if you like to you could start that. apiv3 is probably miles away, or its sooner that I think ?

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 6, 2019

I've already started it. It'll be in 4.0 at least partially

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 6, 2019

It'll be a while to 4.0

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 7, 2019

I added more to the OP.
Oh, also, you may be misunderstanding. There's very little that I will actually remove. This is just a redesign of code and systems to do what we already have better. Currently, we have code all over the place, duplicate processing, etc. and it's making just developing a pain to do. We should have all things that pertain to something in one place, rather than IFs all over the place.

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 7, 2019

The main reasons I haven't worked on Nakamori are:

  • It works for me, so I saw no reason to change anything
  • The current code makes it difficult to change anything without breaking something
  • The things that don't work as intended are API/Server faults, so fixing them didn't require touching Nakamori
  • The new features I want would require too much work doing this first or needs API from Shoko, which we are waiting on APIv3, AniList, etc for.

I use Nakamori daily, so when something stops working, I get more than a little annoyed. I have my new laptop now, so it's easier to develop on. Maybe I'll set up that debugger in pycharm to make all of this easier.

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 7, 2019

@bigretromike you can make a nakamori.lib repo and give me access to it. At the least, I can start doing some things that won't need to be redesigned for the API. Kodi proxies and the object layout, for example

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 8, 2019

Can you make a nakamori.plugin repo, as well? I'm thinking we'll have nakamori be a home folder with submodules pointing to each of the various parts. Then if someone wants to clone, they just git clone --recurse-submodules https://github.com/bigretromike/nakamori and they'll have everything synced up

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 8, 2019

The proper way would be to rename nakamori to nakamori.plugin, then make a new nakamori repo

@bigretromike
Copy link
Owner

bigretromike commented Feb 8, 2019

currently there are many places to point to /nakamori/ lets leave that, home dir can be .plugin which is correct because you want plugin (and all its submodules) if you want to split that into submodules.

I will try to check what points to /nakamori/ and rename those if needed later.

@bigretromike
Copy link
Owner

btw:

I won't try to enforce it, as I've never done it before, but we should have unit testing for all of our directory and API processing. I have no clue how that works in Python yet, so don't panic over it.

I remember like it was before when I wrote that I would like to have test units in nakamori, and you didn't like that idea, but it was a long journey but welcome home ;-)

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 8, 2019

Well it's not like I want to, I just hate debugging in Kodi even more

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 8, 2019

The main reason that I wanted to make nakamori the root is because it'll be easier to zip if each plug-in is in its own folder with nothing else beside it. The nice thing about git is that if you don't like the result, it's easy to revert. I'll do it and you can see what it's like. If you don't like it, then we can revert and make retarded build scripts

@bigretromike
Copy link
Owner

I don't mind. Really. Most of the time you knew what was good, so I'm ok with the proposition.
My concern is only if we need to submodule code inside nakamori? I understand if those submodules will be shared against other plugins, but from what you wrote it wont:

  • plugin - wrapper - that execute script or does it draw gui in kodi ? (not shareable)
  • script - wrapper - that execute lib ? (not shareable)
  • service - stand alone service that will handle player event and other background tasks (not shareable)
  • lib - main computing unit, stand alone script that does the magic (not shareable)

why then you want /nakamori/ to have submodules of those repos when all of them will end as dependency plugin that is needed to run? you wanted to make 1 zip from that via submoduling ?

The only issue I see is how you want to pass tokens/addresses to shoko via that structure, will it pass as argument? (but then you would have to start service only when entering nakamori so it can pass those)

I don't quite understand the different between script and lib, or you want to make script the way it is now, so calendar etc and all the logic would pass via it to lib so you can update lib?

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 8, 2019

No everything depends on lib and resource, but script, plugin, service, etc are not dependent on each other

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 8, 2019

That's part of the goal

@bigretromike
Copy link
Owner

Oh, I now understand, so they all depend on lib being script or something, but if plugin wont depend on service then it should have ability to install it from settings menu to enable community trackers/resume/mark_watched

hope you wont delete the standalone calendar that is part of script I wan't it that way so it can be executed as shortcut from kodi menu without entering nakamori ^_^ also script contains few nifty tools for tweaking calendar. have that in mind. if you make it run as part of plugin with additional execute path for script plugin, sure no problem - but then again icon would be the same. or make calendar as separate plugin?

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 8, 2019

Well, we'll tell it to install together, but the code won't crash without it

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 8, 2019

I'm not deleting anything without running it by you or making it better

@bigretromike
Copy link
Owner

That is understandable, but I would like to emphasize that I want calendar to work the way it does now as launchable script, so it can be put as shortcut in kodi menu.

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Feb 8, 2019

Yep. That won't change

@bigretromike
Copy link
Owner

I'm looking forward this refactor, It will be nice journey

@da3dsoul da3dsoul removed .service APIv3 Only Things that will not aim to be backwards compatible. It will target APIv3 and above only labels Mar 12, 2019
@da3dsoul
Copy link
Contributor Author

Note for later:
Add more proper info for currently playing stuff to the player. I didn't realize it can show more, as we never fed it very much.

@bigretromike
Copy link
Owner

bigretromike commented Mar 14, 2019 via email

@da3dsoul
Copy link
Contributor Author

Currently, at least in this version, we only feed episode info. We can feed series info, as well.

@bigretromike
Copy link
Owner

bigretromike commented Mar 14, 2019

dev-channel is now generated every hour;
we just need to remember to dump version so kodi see it.
3.1.0~beta<number>

@da3dsoul
Copy link
Contributor Author

Okay, @bigretromike your fucking calendar is back. Kodi's built in cast menu is done, too. The search for both cast and series needs done, as well as the custom cast menu. Personally, I'd kind of like to somehow integrate our cast and search systems with kodi and extendscript deeper, but it'll take some looking into.

@bigretromike
Copy link
Owner

heheh yeah I like extendescript too it also bugy as hell. Initially I wanted to make something similar to it because it don't work for most of our cast - it does work with the big names. And we could populate smaller ones on server (shoko) side anyway - we have all the data needed.

@da3dsoul da3dsoul pinned this issue Mar 23, 2019
@da3dsoul
Copy link
Contributor Author

Airing Today and the items above in the checklist are the only things that need to be done. Anything else isn't necessary, but reworking them could be beneficial.

Due to the way that the new login system works, we need a logout button, and means to change the user or connection details from the settings.

@bigretromike
Copy link
Owner

why we need logout ? to reset wizard ? or for what?
Anything else isn't necessary if its not working and was working before its necessary imo

@da3dsoul
Copy link
Contributor Author

We need logout because the good_ip, good_port, wizard, etc settings are unused. It doesn't automatically try to switch users if you enter login details. It just needs to be cleaned up in the settings dialog.

By isn't necessary, I meant could be pushed back a bit and isn't needed for the first release. If I deem it not necessary, then it's generally because I plan to put more work into it than a simple reimplementation.

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Apr 1, 2019

Next, I'll do the What's New dialog and Menu Headings. I might do cast and cast search, or I might put it off until I can actually do it nicely.

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Apr 1, 2019

Oh, nvm. I forgot about logout. I'm doing that next.

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Apr 2, 2019

Connection and Login Wizard from settings is done. What's New needs to be done. Menu Headings are going to be kind of a pain, I'm guessing. I never did get them quite how I wanted them, and I had to modify my skin to show it nicely.

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Apr 2, 2019

How much do you care about Play from Here? Personally, I would like to put it off a bit until we get a better system for service, cache, etc. set up. Then we can implement proper playlist support. It might just be me, but Play from Here and Cast were not that important for daily use, just nice.

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Apr 2, 2019

We are getting very close to calling this ready for a proper beta and maybe release. There are things that I plan to be missing on first release, not because I don't think that they should exist, but that they should be improved first.

@bigretromike
Copy link
Owner

Play from Here never used that, I dont even know what is the different between that and Play.
Cast wasn't that useful until I needed that one cast xD its must be, because of how ExtendedInfo works 👍

Sure, beta if fine and all but stable release need to include more than 90% of things that worked before, with small margin of things that we throw away because no one use them.

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Apr 2, 2019

I'm saying we are basically to that 90%. There's a few more things I'd like to try to do better first, but they are more new stuff than old stuff.

Playlist Mode is Play from Here. Playlist Mode was not a proper name for it. It would play the selected item and everything after it. That's what Play from Here does.

The big one that needs reimplemented is Airing Today, then we've got basically everything that I remember, other than the things I mentioned wanting to push back a bit for redesign.

@bigretromike
Copy link
Owner

Playlist Mode worked on playlist before, but I didn't grasp it in time, and it was throwing errors after that, so I quick-dirty change that to play episode after episode. In one point of time it used Playlist.

@da3dsoul
Copy link
Contributor Author

da3dsoul commented Apr 2, 2019

I see. Well, where it left off, it was basically Play from Here. It sounds like you agree that putting it off to do it right is okay.

After I get a release-worthy state together, I plan to move on to either #305 or #303 and #304, depending on how it goes when I get started. The big part is getting service implemented as a proper way to be a faster means of persistence in memory. Sure, we could use an SQLite DB for persistence, but it's not fast enough for some things, and it's just cumbersome for others. We don't need all playlists to persist between sessions after all.

It would be really cool if we could implement playlists in such a way that Play Next (the addon) could read it. I've always thought it was cool, but didn't care enough to try to get it to work in Nakamori.

@bigretromike
Copy link
Owner

bigretromike commented Apr 2, 2019

Imo if you want to try #303 there need to be done TONS and I mean TONS of testing, because using my PoC with sockets could end up like shoko sometimes (lock up sockets) - as cool as it could be, it was just a PoC if you make up other way to communicate between addon and service then its ok (maybe there is a signal addon). (Edit: or use SQL/Shoko to store data)
#304 is the least important imo, its cool - maybe for super big collection and/or maybe for eigakan/vpn users.
#305 is the most interesting and it would make a great core for the APIv4 because we could just replace endpoints etc.
But pick what you like the most, no forcing you or anything.

Yes, playlist can wait till we do it proper and it would benefit from #303 (if it wont persist in kodi memory). I fix that once because I was happy that someone used it 👍 and wanted to make it more usable hahah.

@bigretromike
Copy link
Owner

bigretromike commented Apr 16, 2019

  • Search options (missing add, clear - I can search them but can't remove them)
  • when searching (for NEW, not picking from list) word that don't result in anything i get popup 'try a different query' and nakamori exit to Videos view (ex. jooj, it don't return jojo, empty list, boom you are in videos)

@da3dsoul da3dsoul reopened this Apr 16, 2019
@da3dsoul
Copy link
Contributor Author

Accidentally hit close while scrolling...
Anyway, remove is in the context menu where it should be.

Jooj vs Jojo is a server error that I won't be fixing for a while. It closing Nakamori is a bug that I'll fix

@bigretromike
Copy link
Owner

bigretromike commented Apr 16, 2019 via email

@bigretromike
Copy link
Owner

It was a really great journey with this Issue. Best one of them all.
You did awesome work but know, the issue need to be closed.
Goodbye AWESOME Issue Ticket.

@bigretromike bigretromike unpinned this issue May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core A core change and a lot of work is needed Enhancement Improving on what's there Kodi Agnostic It does not involve any specific version of Kodi
Projects
None yet
Development

No branches or pull requests

2 participants