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

Core - StorageFile #146

Closed
wants to merge 62 commits into from
Closed

Core - StorageFile #146

wants to merge 62 commits into from

Conversation

EmmmaTech
Copy link
Contributor

@EmmmaTech EmmmaTech commented Mar 9, 2021

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:

  • Nothing for now!
  • Stop committing changes every 5 minutes and actually test a commit before committing (suggestion from natinusala)

What's finished:

  • Fix bug where values from ListStorageObject are added twice when reading from file
  • Re-add ListStorageObject
  • Rewrite the code to fit the original design more

@natinusala
Copy link
Owner

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

Okay, sounds good!

@EmmmaTech
Copy link
Contributor Author

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

@EmmmaTech
Copy link
Contributor Author

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

@EmmmaTech EmmmaTech marked this pull request as ready for review March 10, 2021 02:31
@EmmmaTech
Copy link
Contributor Author

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

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

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

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

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)

@natinusala natinusala mentioned this pull request Mar 15, 2021
@natinusala natinusala added enhancement New feature or request api design labels Mar 15, 2021
@EmmmaTech
Copy link
Contributor Author

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

@EmmmaTech
Copy link
Contributor Author

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

@natinusala
Copy link
Owner

image

What did you do 👀

@EmmmaTech
Copy link
Contributor Author

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

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

@EmmmaTech
Copy link
Contributor Author

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

@natinusala
Copy link
Owner

Rebase is the right choice when making a PR

@EmmmaTech
Copy link
Contributor Author

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

@natinusala
Copy link
Owner

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

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.

See natinusala#140 in the upstream repo for more information.
It checks in the brls.json file for a key called "appname" and sets the config folder to, for example, /config/borealis_demo
If the client is on Switch, then config_folder is set to /config/borealis_demo. If it's on PC, then config_folder is set to ./config/borealis_demo. (Where . is the folder where the executable is)
EmmmaTech and others added 5 commits March 22, 2021 15:42
Tested via the GLFW Platform. Finally was able to compile and run Borealis on macOS.
Macros - Added and changed up the macros

BasicStorageFile - Switched from using vectors to maps

BasicStorageFile - Slightly changed some of the logic in writeToFile

BasicStorageFile - Removed the createStorageObject functions
Demo - Changed Font Size for the text
Util - Fixed the conversion functions
borealis.hpp - Added Util.hpp
Now I love VSCode more.

BasicStorageFile - Added statement to clear up an already used allStorageObjects map (might've caused a segmentation fault)
@EmmmaTech
Copy link
Contributor Author

Well, it happened again. :(

@EmmmaTech
Copy link
Contributor Author

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

@EmmmaTech EmmmaTech changed the title Adding StorageFile class Core - Adding StorageFile concept Mar 24, 2021
BasicStorageFile - Fixed bug where the app would crash when the config folder doesn't exist

BasicStorageFile - Fixed bug where the saving document part never got it's error code

SettingsFile - Changed from class to struct
BasicStorageFile - Changed error handling logic with saving

Macros - Changed the name of some parameters to make it less confusing
@EmmmaTech
Copy link
Contributor Author

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

@EmmmaTech EmmmaTech changed the title Core - Adding StorageFile concept Core - StorageFile Nov 7, 2021
@EmmmaTech
Copy link
Contributor Author

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 EmmmaTech mentioned this pull request Jun 5, 2022
@EmmmaTech
Copy link
Contributor Author

Reopened in #175 due to branch reorganization.

@EmmmaTech EmmmaTech closed this Jun 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api design enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants