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

WIP: Feature/joonne/data storage #37

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

joonne
Copy link
Owner

@joonne joonne commented Jul 30, 2019

  • Sketching the data storage with JSON serialization
  • Implemented first version of Resume Functionality #11 already as a proof on concept

@joonne joonne requested a review from mlehtima July 30, 2019 20:10
@joonne
Copy link
Owner Author

joonne commented Jul 30, 2019

wdyt? I'm just testing around, wanted to get lighter data storage implementation and I thought JSON would serve us well enough as the data model?

… can be detected easily if the first onChange is from that.
@@ -33,6 +33,10 @@ Page {
if (visible) updateCover(qsTr("Now playing"), program.title, program.itemTitle)
}

Component.onDestruction: {
appWindow.insertStartedProgram({ id: program.id, progress: mediaPlayer.position })
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check if this is called upon closing the app by a swipe and make sure this is called also when pause button is pressed in case the handle isn't called upon closing the app forcefully

@joonne joonne marked this pull request as ready for review August 20, 2019 18:03
@@ -41,6 +45,7 @@ Page {
subtitlesText.getSubtitles(subtitlesUrl)
}
mediaPlayer.source = response.url
if (appWindow.state.startedPrograms[program.id]) mediaPlayer.seek(appWindow.state.startedPrograms[program.id])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could be more readable if this had curly brackets and function call inside those or at least an empty line after this because otherwise it's possible to read that the if affects the next line. Also the line is quite long now.

#include <QTextStream>
#include <QStandardPaths>
#include <QDir>
#include <QDebug>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make the include list alphabetically ordered.

Q_INVOKABLE QJsonObject getState();

signals:
public slots:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to have the empty signals and public slots defined here?

QFile file(this->getPath());

if (!file.open(QIODevice::ReadWrite))
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move curly bracket to same line with if which is the style used in other files. Same for other if statements in the new code.

@mlehtima
Copy link
Collaborator

I also wonder about the efficiency of the code because both setState and getState make copies of the state. Need to check the code more carefully and also investigate is compiler will optimize those situations to something more efficient.

@joonne joonne changed the title Feature/joonne/data storage WIP: Feature/joonne/data storage Nov 7, 2020
@joonne joonne marked this pull request as draft November 7, 2020 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants