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: exit app when closing last tab #601

Merged
merged 21 commits into from
Jan 27, 2023
Merged

Conversation

soumyamahunt
Copy link
Contributor

@soumyamahunt soumyamahunt commented Jun 24, 2020

Implemented functionality to exit app when last tab is closed as requested in #248.

PR Type

What kind of change does this PR introduce?

  • Feature

Other information

The implemented method doesn't interfere with Windows' ability to remember window size and position.

W13JDdBKCD

M3mIjjDjVO

@0x7c13
Copy link
Owner

0x7c13 commented Jun 24, 2020

Never realized there is a method called "TryConsolidateAsync" and does exactly what is needed. How did you figure that out?

@soumyamahunt
Copy link
Contributor Author

Never realized there is a method called "TryConsolidateAsync" and does exactly what is needed. How did you figure that out?

Just by reading documentation.

@0x7c13
Copy link
Owner

0x7c13 commented Jun 24, 2020

Since it has been too long that I changed this behavior (it used to be like this). Maybe we should add this as an option in Advanced Settings? Default would be not to close the app and let's give the option to user.

@soumyamahunt
Copy link
Contributor Author

Since it has been too long that I changed this behavior (it used to be like this). Maybe we should add this as an option in Advanced Settings? Default would be not to close the app and let's give the option to user.

Made changes as you asked, here is how the ui looks:

ApplicationFrameHost_NvKe4ul4Ys

@avgrad
Copy link

avgrad commented Jun 25, 2020

I had a go at that myself, but didn't know about TryConsolidateAsync(), very nice.
+1 for merging this.
I'll happily supply German translations when this gets to master.

@soumyamahunt
Copy link
Contributor Author

@Jasonstein is there any further work you need from me here or is it RTM??

@0x7c13
Copy link
Owner

0x7c13 commented Mar 22, 2021

@Jasonstein is there any further work you need from me here or is it RTM??

image
I don't like the translation here since it is not very user-friendly (maybe just App Preference and Exit when closing the last tab?). Let me think of any better wording here before merging the PR. Btw I will probably put this in the next release.

@avgrad
Copy link

avgrad commented Mar 22, 2021

Some Suggestions for the wording:

  • "App Behavior Preferences"
  • "Window Behavior Preferences"
  • "Exit app when closing last tab"

@0x7c13
Copy link
Owner

0x7c13 commented Mar 22, 2021

  • "Window Behavior Preferences"
  • "Exit app when closing last tab"

LGTM. @soumyamahunt thoughts?

@soumyamahunt
Copy link
Contributor Author

  • "Window Behavior Preferences"
  • "Exit app when closing last tab"

LGTM. @soumyamahunt thoughts?

Fine with this, but I think "Always open new window" conveys better meaning than "Window Behavior Preferences".

@soumyamahunt
Copy link
Contributor Author

This is how it looks now after the changes:

ApplicationFrameHost_lBRJmctwa3

@avgrad
Copy link

avgrad commented Mar 23, 2021

The description below the switches is confusing. Its a single sentence, describing two independent settings. When I read it, it appears to me, that "closing the last tab" will also somehow enable "always open in new window".

I think it should be split up to describe the options seperately, or maybe we should just omit the description of "Exit app when closing last tab", since the title explains it well enough.

@soumyamahunt
Copy link
Contributor Author

The description below the switches is confusing. Its a single sentence, describing two independent settings. When I read it, it appears to me, that "closing the last tab" will also somehow enable "always open in new window".

I think it should be split up to describe the options seperately, or maybe we should just omit the description of "Exit app when closing last tab", since the title explains it well enough.

Yes this could create confusion, I have removed description for last tab closing option.

ApplicationFrameHost_2KwZd6pcKg

@soumyamahunt soumyamahunt changed the title Closing the last tab now exits the app feat: Exit app when closing last tab Mar 29, 2021
@UmbralReaper
Copy link

What about moving the description to beneath the first switch, and have the description change depending on whether the switch is on or off. If the switch is on it reads The app is automatically closed when there are no open tabs.. If the switch is off it reads The app stays open when there are no open tabs.

@soumyamahunt
Copy link
Contributor Author

What about moving the description to beneath the first switch, and have the description change depending on whether the switch is on or off. If the switch is on it reads The app is automatically closed when there are no open tabs.. If the switch is off it reads The app stays open when there are no open tabs.

Notepads doesn't follow different content depending on whether switch is on or off. Doing that only for this will create confusion. Also, I personally think this type of changing content whether switch is on/off creates more confusion for user. In your example when the user toggles it on it reads The app is automatically closed when there are no open tabs. does that mean currently this behavior applied or when user toggles switch off the behavior is applied, which creates confusion.

@UmbralReaper
Copy link

Notepads doesn't follow different content depending on whether switch is on or off. Doing that only for this will create confusion. Also, I personally think this type of changing content whether switch is on/off creates more confusion for user.

I agree. I honestly don't know what I was thinking.

Regardless, the description text should be moved to under the button it corresponds to.

@soumyamahunt
Copy link
Contributor Author

Regardless, the description text should be moved to under the button it corresponds to.

Currently the new toggle switch doesn't contain any description as the content itself pretty self explanatory, as for the "always open new window" option the description for it is below it.

@UmbralReaper
Copy link

Currently the new toggle switch doesn't contain any description as the content itself pretty self explanatory, as for the "always open new window" option the description for it is below it.

Turns out I was reading it the wrong way round 😅 .

@soumyamahunt soumyamahunt changed the title feat: Exit app when closing last tab feat: exit app when closing last tab Apr 18, 2021
@JTLee98 JTLee98 mentioned this pull request Jun 23, 2021
@LucaSorvillo
Copy link

Why has this functionality not been applied yet?

@philippehawi
Copy link

Any updates on this? Will it be added to a release soon?

@0x7c13 0x7c13 merged commit ca49f7f into 0x7c13:master Jan 27, 2023
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.

6 participants