natinusala / borealis

Hardware accelerated, controller and TV oriented UI library for PC and Nintendo Switch (libnx)
Apache License 2.0
266 stars 83 forks source link

Core - StorageFile #146

Closed EmmmaTech closed 2 years ago

EmmmaTech commented 3 years ago

Check #140 for context clues

This is my implementation of the StorageFile class idea, for the release of Borealis. This is a work-in-progress Pull Request since I just started on this.

To-Do with this Pull Request:

What's finished:

natinusala commented 3 years ago

Hey thanks for the contribution!

I will have a look once I'm done with touch and the RetroArch stuff I'm currently busy wtith.

EmmmaTech commented 3 years ago

Okay, sounds good!

EmmmaTech commented 3 years ago

I didn't do a compilation test until now, and there were some compiler errors. So I went ahead and fixed them up.

EmmmaTech commented 3 years ago

The class in its simplest, probably functional form is finished. I'm open for a review now.

EmmmaTech commented 3 years ago

There are a lot of issues with the class (i.e. the writing and reading functions not working correctly) so I'm going to try and fix them.

EmmmaTech commented 3 years ago

So I have a new idea. Instead of using my approach from yesterday, I can use a new one that should bring this implementation of this idea closer to the original idea. There will be a new class called StorageObject, which contains variables for the value, name, and type (like int or string for example).

In StorageFile, there will be a whole std::vector containing these StorageObjects. This means readFromFile can read from the std::vector, not directly from the XML file. writeToFile should be the same, and there will be a new function to parse the XML file into StorageObject objects.

I was also thinking of implementing the save function with individual elements, but that might require more work.

natinusala commented 3 years ago

I'm pretty sure what I wrote in the issue can be implemented exactly as I wrote it, it's a design more than an "idea"

EmmmaTech commented 3 years ago

Yeah, I think I can do it if I put more effort. Right now, you don't have to write a lot of code, but you might still have to write code.

EmmmaTech commented 3 years ago

I just confirmed, and now everything works as intended! I had to fix up some runtime errors mostly relating to how I was treating pointers. (I was treating them more like a normal variable than a pointer, there were many Segmentation Faults)

EmmmaTech commented 3 years ago

Wow, I messed up. Tried to get the latest changes from the upstream main branch.

EmmmaTech commented 3 years ago

Actually wait, I didn't, but it's a big mess

natinusala commented 3 years ago

image

What did you do 👀

EmmmaTech commented 3 years ago

I did a git rebase instead of git merge...

I wanted to update my fork since there was a new commit.

EmmmaTech commented 3 years ago

Yes, I literally don't know how to properly use git. I use GitHub Desktop most of the time

natinusala commented 3 years ago

Rebase is the right choice when making a PR

EmmmaTech commented 3 years ago

Yeah, but I'm not sure why it recommitted everything that I already committed to this Pull Request

natinusala commented 3 years ago

Because when you rebase it removes all your commits, updates the branch and re-applies your commits back on the updated branch. As a consequence, your commits are not the same as the previous ones (same content but new hash). The history has also been rewritten, making a force push mandatory. So since the commits are all same same, but different, but still same, GitHub treats them all as new commits in the PR.

EmmmaTech commented 3 years ago

Actually, yes, I should've done a force push. I pushed directly through GitHub Desktop, so it must've not done a force push. I was thinking of reverting to the previous commit before I rebased, and try again.

EmmmaTech commented 3 years ago

Well, it happened again. :(

EmmmaTech commented 3 years ago

Well actually, I fixed it! But it counted all the commits from today. And it didn't update from the upstream

EmmmaTech commented 3 years ago

Alright, I have finally tested on the Nintendo Switch! It seems to work fine, just the weird segmentation faults sometimes.

EmmmaTech commented 2 years ago

So because your issue was a "design", not an "idea", I decided to rewrite the entire PR to match the design a lot more. It has some missing functions from the original design, but otherwise, it works fine.

EmmmaTech commented 2 years ago

Reopened in #175 due to branch reorganization.