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

Rewrite WelcomePage #186

Merged
merged 10 commits into from
Apr 9, 2020
Merged

Rewrite WelcomePage #186

merged 10 commits into from
Apr 9, 2020

Conversation

danirabbit
Copy link
Member

@danirabbit danirabbit commented Apr 7, 2020

  • Inline namespace
  • GObject style
  • Make sure we create buttons before we try to change their visibility by adding signals at the end
  • Reduce variable scope where possible
  • Update replay button title and subtitle when last-stopped changes
  • Update replay button visibility and launch the correct file when current-video changes
  • DRY

@danirabbit danirabbit requested a review from arshubham April 7, 2020 21:16
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

See inline comment

src/Widgets/WelcomePage.vala Show resolved Hide resolved
@danirabbit danirabbit requested a review from jeremypw April 8, 2020 19:05
@jeremypw
Copy link
Collaborator

jeremypw commented Apr 9, 2020

It still seems to suffer the issue in #187. Are you not getting that issue? The problem seems to be that the "last-stopped" setting changes when the window is destroyed from "0" to "0.002..." for some reason. A quick fix would be to change the test in update_replay_title () from> 0 to > 0.003 or change the test in the destroy callback. Note that this issue only seems to occur with_short_ videos (e.g. 1 minute)

@danirabbit
Copy link
Member Author

I'm not experiencing and issue with the time save being set to a non-zero value on destroy.

I think this sounds like a bug in a different part of the codebase though and I'm hesitant to add a workaround here instead of solving it at the source

@jeremypw
Copy link
Collaborator

jeremypw commented Apr 9, 2020

OK - it can be left for another PR then.

@jeremypw jeremypw merged commit 54a6268 into master Apr 9, 2020
@jeremypw jeremypw deleted the welcome-page-rewrite branch April 9, 2020 18:26
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