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

feat(mainwindow): update saving logic & detect text change #44

Merged
merged 8 commits into from
Dec 15, 2019

Conversation

ouuan
Copy link
Member

@ouuan ouuan commented Dec 13, 2019

This fixes the second part of #23.

It's a big change, I'm not so sure that there aren't any mistakes. Please check it carefully.

BTW, the code reuse is terrible. There are too many copy pastings, especially the 3 inputs, 3 outputs, 3 runners part.

@caretaker-claire
Copy link

Hi ouuan
Thank you for your contribution to cp-editor2. I will request a review from one of the contributor. Once they approves and all CI checks passes, I will merge your Pull Request.

@coder3101
Copy link
Member

Yes lot of code was inherited from my old editor. Which was for personal and fun use! I will tr making it more organised !!

@ouuan
Copy link
Member Author

ouuan commented Dec 13, 2019

Something should be changed in favor of launchCompanionSession.

@coder3101
Copy link
Member

Something should be changed in favor of launchCompanionSession.

What?

@ouuan
Copy link
Member Author

ouuan commented Dec 13, 2019

Something should be changed in favor of launchCompanionSession.

What?

I will fix it. Left a comment here for:

  1. self reminding;
  2. don't merge.

@coder3101
Copy link
Member

When your work is done, mention me to merge this work!

1. fix auto-save nullptr
2. use closeChangedConfirm for opening a file
3. different messages when saving via different ways
It sometimes crashes when request is received, but I haven't found the reason.
@coder3101
Copy link
Member

Application terminates because it thinks that no window is visible.

Read this problem and in the problem itself is the answer.

Basically what you have to do is call app.setQuitOnLastWindowClosed(false) in main as soon as app is created.

Read this answer https://stackoverflow.com/questions/5116459/problem-with-hidden-qmainwindow-application-crashes-after-qmessagebox-is-displa

@ouuan
Copy link
Member Author

ouuan commented Dec 14, 2019

Application terminates because it thinks that no window is visible.

Read this problem and in the problem itself is the answer.

Basically what you have to do is call app.setQuitOnLastWindowClosed(false) in main as soon as app is created.

Read this answer https://stackoverflow.com/questions/5116459/problem-with-hidden-qmainwindow-application-crashes-after-qmessagebox-is-displa

The bug still happens:

int main(int argc, char* argv[]) {
  QApplication a(argc, argv);
  a.setQuitOnLastWindowClosed(false);
  QStringList args = a.arguments();
  QString filePath = args.size() > 1 ? args[1]: "";
  MainWindow w(filePath);
  w.show();
  return a.exec();
}

@ouuan
Copy link
Member Author

ouuan commented Dec 14, 2019

How about this: open a new session when it's an unchanged temporary buffer (i.e. unsaved, the content is exactly the template), otherwise don't open a new session.

Open a new session when it's an unchanged temporary buffer (i.e. unsaved, the content is exactly the template), otherwise don't open a new session.
@ouuan
Copy link
Member Author

ouuan commented Dec 14, 2019

@coder3101

It looks good to me now, you can merge it after checking again.

@coder3101
Copy link
Member

After i merge it today, i will release a pre-release version, can you guarantee that New session won't crash application?

@ouuan
Copy link
Member Author

ouuan commented Dec 15, 2019

After i merge it today, i will release a pre-release version, can you guarantee that New session won't crash application?

It didn't crash when I tested it, and in this version, there is no message box opened in the backstage.

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.

None yet

2 participants