huynhsontung
Add launch page settings
Add a setting and surrounding infrastructure to let users pick which page should appear when they launch the app (except when launching via file). Any page besides the settings page is supported, including second-level pages like the albums or artists pages of the music library.
Nice change. I am not so sure about the subpages, we might want to consider adding only the navigation view items. That approach would allow us to reuse them for a eventual jumplist support.
Also you were only required to add the English string, as Crowdin manages the rest. Could you please revert those changes?
I did consider adding only the navigation view items, but I found that I didn't like this approach because it effectively forces on the user an arbitrary opinion of the app about which subpage is more important. For me personally, the default songs view of the music page is not useful, and I always switch to the albums view. This is an arbitrary preference on my side, but so it would be for the app to only let me launch at the songs view. It would leave the idea of this feature, to let the user control where to launch the app, only half-baked. If you don't share the app's opinion on which subpage is the most important, you're left with an extra navigation step to do each time.
Finally, on the default Media Player app of Windows, the behavior is that while there is no setting like I'm implementing, the app does remember the last page you had open, down to the subpage level. I didn't want Screenbox to be less-than in this regard. :)
Regarding localization, should I just revert the changes to all Resources.resw files except for en-US? I'm a native German speaker so I already added de-DE translations, would it make sense to keep those rather than going through Crowdin for them?
I did consider adding only the navigation view items, but I found that I didn't like this approach because it effectively forces on the user an arbitrary opinion of the app about which subpage is more important.
It’s cool, I was just throwing my opinion out there.
Finally, on the default Media Player app of Windows, the behavior is that while there is no setting like I'm implementing, the app does remember the last page you had open, down to the subpage level. I didn't want Screenbox to be less-than in this regard. :)
We should do the same at some point, same for the jumplist support.
Regarding localization, should I just revert the changes to all
Resources.reswfiles except for en-US? I'm a native German speaker so I already added de-DE translations, would it make sense to keep those rather than going through Crowdin for them?
Yes, the de-DE translation should be reverted as well, and yeah it's a bit of a pain. Crowdin will overwrite any Resources.resw except for the en-US. Sorry about that.
I have reverted all the translations. I suppose I can only add the de-DE translation back through Crowdin after this PR has been merged?
I have reverted all the translations. I suppose I can only add the de-DE translation back through Crowdin after this PR has been merged?
Thanks. Yes, and if you don't have an account, or don't want to create one, we can translate it.
@huynhsontung Would you like me to implement Copilot's suggested changes? Other usages of ArgumentOutOfRangeException in the code base do not set an error message so I didn't implement this either, but I can do it if you'd like.
@huynhsontung Would you like me to implement Copilot's suggested changes? Other usages of
ArgumentOutOfRangeExceptionin the code base do not set an error message so I didn't implement this either, but I can do it if you'd like.
Can't speak for him but (IMHO) the ArgumentOutOfRangeException changes aren't strictly necessary, don't go out of your way to implement them.
Please ignore the Copilot comments and suggestions. I was just testing the feature.
I noticed that you also include subpage in the settings. I want to implement subpage persistence so when a nav page is selected it goes to the last subpage, as you noted with the Media Player. In the meantime, this setting will do the job.
Sorry for the delay in reviewing. I have been extremely busy.
Hi @huynhsontung, any chance we could revisit this? I think the check failed only because it used the deprecated Windows Server image, which I saw you fixed in the meantime.
I understand your concerns. So, if I understand you right, you'd rather scrap this implementation and go with an entirely different approach, correct? In that case I would close this. I might be able to get around to the approach you propose if I find the time and then I can create a separate PR for that.