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

Dialog to select proposal/directory when starting GUI #9

Merged
merged 18 commits into from
Jul 6, 2023

Conversation

takluyver
Copy link
Member

image

I haven't tried to do any refactoring to take full advantage of this yet.

Copy link
Member

@JamesWrigley JamesWrigley left a comment

Choose a reason for hiding this comment

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

Things we discussed:

  • Adding tests
  • Open dialog from menu entry
  • Creating a backend from the dialog if one doesn't exist

damnit/gui/open_dialog.py Outdated Show resolved Hide resolved
damnit/gui/open_dialog.py Outdated Show resolved Hide resolved
@takluyver
Copy link
Member Author

Thanks @JamesWrigley for the review. I've made all the changes we discussed now; do you want to take another look overt the latest changes? The main change, other than the tests, is to the code for creating a DAMNIT DB and starting the backend if they don't already exist.

@JamesWrigley
Copy link
Member

Hmmm, now I'm getting crashes when testing this on Maxwell and my local machine 🤔 Both with selecting by proposal and directory:

(amore) ➜  mid-2832 git:(open-db-dialog) amore-proto gui --no-kafka    
QThread: Destroyed while thread is still running
[1]    3384071 IOT instruction (core dumped)  amore-proto gui --no-kafka

@takluyver
Copy link
Member Author

Gah, threads. I saw those errors while running the tests and fixed them there, but then didn't try again with the actual application. I'll take a look.

@takluyver
Copy link
Member Author

OK, I think I've finally got the application, the local tests and the CI all working 😅 🤞 .

Copy link
Member

@JamesWrigley JamesWrigley left a comment

Choose a reason for hiding this comment

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

I didn't have quite enough time to test it myself, but I trust you and CI 😁 LGTM!

(P.S. I might also suggest squashing some of the commits before merging, but I leave that up to you)

@takluyver takluyver merged commit dbb4456 into master Jul 6, 2023
1 check passed
@takluyver takluyver deleted the open-db-dialog branch July 6, 2023 15:40
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