-
Notifications
You must be signed in to change notification settings - Fork 76
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
Massive backend/indexing improvements #159
Massive backend/indexing improvements #159
Conversation
This is just a proof of concept, this isn't 100% finalized yet and thus you will see duplicate files in the source, comments on code that doesn't exist etc... so please don't make a pull request of this branch in this stage.
* Prevent crashes when playing media in case the app doesn't have access to it, or another error occurred * No description * Remove Segoe UI Variable
…Software/Rise-Media-Player into feat/indexing-improvements
(Proper review needed before merge, so please don't merge) |
Code looks good to me, although I'm unable to test it as of now. Will share any updates here as soon as I'm able to. |
Seems to work okay. Images for props, setup, playlists arent showing up though. Might be on my end :/ |
I'll investigate it tomorrow, can't do anything anymore rn. |
@josephbeattie every image works for me, not sure why it wouldn't work for you, perhaps you're on an old branch or you accidentally deleted the files? (note: this result was from the indexing improvements branch, which is what this PR uses right now) |
That’s really strange. Haven’t changed branches and all the images are there in the files. Certain images (like the main titlebar icon and widgets icon) show up. This is weird I’m going to try to reclone |
…r make the song duplicate
…cation. Thanks @esibruti for noticing!
…cation. Thanks @esibruti for noticing!
…Software/Rise-Media-Player into feat/indexing-improvements
In the Repository.cs file you could just use a common base class (e.g DbObject) me thinks, for upsert, delete, etc |
For my part, everything is working. Just one thing: is it possible to change the name of the Rise Media Player Dev folder to Rise.Uwp? What about Rise Media Player Dev.sln to RiseMediaPlayer.sln? I think it would be better because files and folders shouldn't contain spaces. contributing.md -> CONTRIBUTING.md |
@esibruti I do agree with you, but that'd break existing clones, so maybe at a later date. |
@YourOrdinaryCat I'm doing this test and I was able to change the names without causing any problems. I have a draft pull request that shows this |
Well you did too many breaking changes so I'll have to close the PR to ensure compatibility with existing branches/forks. |
No problem, I was just testing to see what would happen if I did such a thing. As it says there, just for testing |
@itsWindows11 was trying to improve GetListsAsync, but I'm running into an exception. It's in the songs page code and related to AdvancedCollectionView, so it'll take a while to get fixed. Should we merge this now? None of the commits up until this point are problematic I think, and most of the new stuff seems to be finished. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I forgot the review, code looks good.
Okies, I could merge. |
Songs shouldn't duplicate anymore, and the new backend will be much easier to maintain (its just one, ONE file!).