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: set default preview when stream can't be found anymore #181

Merged
merged 14 commits into from
May 30, 2023

Conversation

BeierKevin
Copy link
Contributor

@BeierKevin BeierKevin commented May 26, 2023

PR description

Describe your changes in detail here

If you have a program like Notepad or any other application open and selected for preview, when you close the Notepad application, the stream handler will react to it by fetching the capturable sources again and setting the main screen as the default preview. If you close Notepad and it is not the current preview, it will only trigger a refetch for the sources, ensuring that the sources tab with screens and applications is always up to date.

However, this does not happen when you open an application. I believe it is not possible with Electron to trigger a refetch automatically when opening an application. In such cases, you can simply press the refresh button, which does the job and should be sufficient.

Additionally, I added workspace settings which we should have done probably a lot earlier, which formats the code on save for everyone should be enabled per person. This was not part for this issue but noticed it and thought why not just add it.

Definition Of Done (DoD)

This PR can be squashed / merged if

  • a developer is assigned
  • the PR is NOT estimated
  • the PR is labeled
  • the PR is NOT assigned to the current sprint
  • a meaningful title has been set according to https://www.conventionalcommits.org/
  • the PR is described in detail
  • the PR links to an issue
  • the PR has been reviewed

Add additional conditions here if necessary for this PR

fix: #71

@BeierKevin BeierKevin requested a review from Claiyc May 26, 2023 21:28
@BeierKevin BeierKevin self-assigned this May 26, 2023
@BeierKevin
Copy link
Contributor Author

BeierKevin commented May 26, 2023

I think this implementation satisfies what the issue #71 suggests but it would be possible to rewrite the streamhandler like a composable and utilise Vue related functionalities like watching etc. to improve performance and the user experience with everything. Could be a harder issue to tackle, tho.

And more like writing an composable instead of an singelton

@Claiyc
Copy link
Member

Claiyc commented May 30, 2023

Attached issue seems to be solved. However upon closer examination of the source capturing, I ran into quite a few issues with capturining specific applications. The actual applications are often cut off / the view does not show the entire application.
Screenshot (2)

Funny thing is that while I drag the selected application or use WIN+SHIFT+S for taking the screenshot, the preview adjusts and shows the entire application (as it should normally).
This issue does not seem related to specific changes in this PR. The same issue occurs on feat/182

@BeierKevin
Copy link
Contributor Author

Huh I did not have these issues or at least didn't try that much out, but is this related to this implementation really or was this already before ?

@Claiyc
Copy link
Member

Claiyc commented May 30, 2023

Huh I did not have these issues or at least didn't try that much out, but is this related to this implementation really or was this already before ?

It was an issue before already

@Claiyc
Copy link
Member

Claiyc commented May 30, 2023

Please resolve conflicts before next review

@BeierKevin
Copy link
Contributor Author

Huh I did not have these issues or at least didn't try that much out, but is this related to this implementation really or was this already before ?

It was an issue before already

Ahh ok

@BeierKevin BeierKevin requested a review from Claiyc May 30, 2023 07:52
Copy link
Member

@Claiyc Claiyc left a comment

Choose a reason for hiding this comment

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

lgtm

@BeierKevin BeierKevin merged commit d2382bb into main May 30, 2023
@BeierKevin BeierKevin deleted the feat/71-previewstream-closing branch May 30, 2023 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Closing current previewstream
2 participants