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

GTK Reflashing & Write Cancellation #53

Merged
merged 13 commits into from
Jul 31, 2018
Merged

GTK Reflashing & Write Cancellation #53

merged 13 commits into from
Jul 31, 2018

Conversation

mmstick
Copy link
Member

@mmstick mmstick commented Jul 18, 2018

  • libpopsicle was modified to enable early killing of the device flashing process
  • state is reset when switching back to the main view in popsicle-gtk
  • popsicle-gtk will revert back to the main view when:
    • Cancel is selected on the device flashing view
    • Back is selected on the summary view
  • popsicle-gtk GTK widgets are now Rc'd together

Closes #51

- libpopsicle was modified to enable early killing of the device flashing process
- state is reset when switching back to the main view in popsicle-gtk
- popsicle-gtk will revert back to the main view when:
  - Cancel is selected on the device flashing view
  - Back is selected on the summary view
- popsicle-gtk GTK widgets are now Rc'd together
authors = ["Jeremy Soller <jeremy@system76.com>"]
authors = [
"Jeremy Soller <jeremy@system76.com>",
"Michael Aaron Murphy <michael@system76.com>",
Copy link
Contributor

Choose a reason for hiding this comment

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

It's about time! 🎉

@brs17 brs17 requested a review from cassidyjames July 18, 2018 22:48
@brs17
Copy link
Contributor

brs17 commented Jul 18, 2018

@cassidyjames asked for your review just cause there are some graphical changes (and it never hurts).

@brs17 brs17 mentioned this pull request Jul 19, 2018
@cassidyjames
Copy link
Contributor

It looks like the back button is marked as destructive. It doesn't need to be.

image

It should probably also say something like, "New Image" or something, since it's not just going back one step but is starting completely over.

Copy link
Contributor

@cassidyjames cassidyjames left a comment

Choose a reason for hiding this comment

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

See comment above

@mmstick
Copy link
Member Author

mmstick commented Jul 19, 2018

@cassidyjames Unless the user is not changing the image, and immediately progressing back to device selection, since the previous image is still selected at that point. In that case, should it be "Flash Again"?

@cassidyjames
Copy link
Contributor

@mmstick that could work

@brs17
Copy link
Contributor

brs17 commented Jul 27, 2018

When canceling a write midway through, the Cancel button remains highlighted red even when back at the first screen.
aftercancel

I would imagine this should dehighlight (to look the way it does originally on the first screen).
firstpage

That also makes me wonder/think if that Cancel button on the first screen should say Close like on the last screen (but on the left side).
flashagain

Thoughts @cassidyjames ?

@brs17
Copy link
Contributor

brs17 commented Jul 27, 2018

Functionality wise, it looks good though.

Just waiting for that last commit to build...

@brs17
Copy link
Contributor

brs17 commented Jul 27, 2018

When I was showing the latest patches to @cassidyjames we noticed that the Flash button is enabled even when a drive is not selected. That should not be the case.

Copy link
Contributor

@cassidyjames cassidyjames left a comment

Choose a reason for hiding this comment

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

To keep the forward (right) position affirmative, I think it would make sense to update it to say, "Done" on the last screen now. I know that's not strictly something that was touched in this PR, but I think with the other changes, it caused me to look a little more closely at the back/forward buttons and their copy.

@cassidyjames
Copy link
Contributor

It would also be great to update screenshots in the repo with this PR so we keep those up to date with the current UI.

@mmstick
Copy link
Member Author

mmstick commented Jul 27, 2018

The summary view now says "Done", and I've fixed the next button on the device selection view. Will add new screenshots.

@mmstick
Copy link
Member Author

mmstick commented Jul 27, 2018

Found an issue that can cause the device selection view to lock if select_all is enabled, and then any device is removed. Looking for the cause of the lock.

@mmstick
Copy link
Member Author

mmstick commented Jul 27, 2018

Screenshots are now updated.

@mmstick
Copy link
Member Author

mmstick commented Jul 27, 2018

Rendered

Copy link
Contributor

@brs17 brs17 left a comment

Choose a reason for hiding this comment

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

Looks good from my side of things. I'm thinking @jackpot51 should code review before we merge.

@brs17 brs17 requested a review from jackpot51 July 30, 2018 14:27
@mmstick mmstick merged commit 3699e1f into master Jul 31, 2018
@jackpot51 jackpot51 deleted the reflashing branch August 1, 2018 14:25
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.

3 participants