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

add theme selection to setup wizard #249

Merged

Conversation

TayouVR
Copy link
Member

@TayouVR TayouVR commented Oct 23, 2022

This moves the UI code for selecting theme, icon theme and background cat from LauncherPage to ThemeCustomizationWidget, which is then embedded in both LauncherPage and ThemeSetupWizard.

TODO:

X currently only displaying some of the widgets doesn't layout perfectly, this doesn't break functionality, but if someone has a way to fix it, I'd greatly appreciate. (related code: https://github.com/PrismLauncher/PrismLauncher/pull/249/files#diff-ee2722496429e9dee7a6ef8fd601be31996920cf7ef55ba3ec6ec8ac90bef2b6R63)

Blocked by #510 Resolved in #600

@TayouVR TayouVR force-pushed the theme-selector-first-time-wizard branch from 9ff89d6 to ad3203f Compare October 23, 2022 20:35
@flowln flowln added the enhancement New feature or request label Oct 24, 2022
@TayouVR TayouVR force-pushed the theme-selector-first-time-wizard branch from ad3203f to 814da66 Compare November 6, 2022 02:35
@TayouVR
Copy link
Member Author

TayouVR commented Nov 6, 2022

oof, my jank connect() fails on Qt5... honestly not all that surprising... was planning to rework that part anyways x3

@TayouVR TayouVR force-pushed the theme-selector-first-time-wizard branch 2 times, most recently from 2f356a7 to b2e8959 Compare November 6, 2022 23:29
@TayouVR TayouVR force-pushed the theme-selector-first-time-wizard branch from b2e8959 to 1cddb21 Compare December 29, 2022 16:03
@TayouVR
Copy link
Member Author

TayouVR commented Dec 29, 2022

image
image
image

ready for review, finally x3

@TayouVR TayouVR marked this pull request as ready for review December 29, 2022 16:08
@flowln flowln self-requested a review December 29, 2022 16:13
Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

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

this is very cool, i love the demo thingies on the setup wizard 😍

the theme ignoring issue however is very critical, my dark theme doesn't get applied when i open up the launcher 😭

launcher/ui/setupwizard/ThemeWizardPage.cpp Outdated Show resolved Hide resolved
launcher/ui/setupwizard/ThemeWizardPage.cpp Outdated Show resolved Hide resolved
launcher/ui/widgets/ThemeCustomizationWidget.h Outdated Show resolved Hide resolved
launcher/ui/widgets/ThemeCustomizationWidget.h Outdated Show resolved Hide resolved
launcher/ui/themes/ThemeManager.cpp Outdated Show resolved Hide resolved
launcher/ui/widgets/ThemeCustomizationWidget.cpp Outdated Show resolved Hide resolved
launcher/ui/widgets/ThemeCustomizationWidget.h Outdated Show resolved Hide resolved
launcher/ui/setupwizard/ThemeWizardPage.cpp Outdated Show resolved Hide resolved
launcher/Application.cpp Outdated Show resolved Hide resolved
launcher/Application.cpp Outdated Show resolved Hide resolved
launcher/ui/themes/ThemeManager.cpp Outdated Show resolved Hide resolved
launcher/ui/themes/ThemeManager.cpp Outdated Show resolved Hide resolved
@TayouVR TayouVR force-pushed the theme-selector-first-time-wizard branch from 760745d to 9fbf70c Compare January 4, 2023 13:44
Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot

@flowln flowln self-requested a review January 7, 2023 16:31
@HyperSoop
Copy link
Contributor

Why would all new users need to customize their cat though? seems like a weird thing to preview and emphasis a literal easter egg.

@DioEgizio
Copy link
Member

Why would all new users need to customize their cat though? seems like a weird thing to preview and emphasis a literal easter egg.

why not? it's not really an easter egg at this point either

@HyperSoop
Copy link
Contributor

Why would all new users need to customize their cat though? seems like a weird thing to preview and emphasis a literal easter egg.

why not? it's not really an easter egg at this point either

It seems like something normal on the inside of this community but someone coming to use the software from the outside would see this as very very weird and unfitting.

@DioEgizio
Copy link
Member

Conflicting with develop

@TayouVR
Copy link
Member Author

TayouVR commented Jan 9, 2023

Why would all new users need to customize their cat though? seems like a weird thing to preview and emphasis a literal easter egg.

I couldn't get the cat dropdown to disappear without leaving a gap, so I decided to just let ppl choose the cat too..

@HyperSoop
Copy link
Contributor

HyperSoop commented Jan 9, 2023

this is seriously unacceptable though. the cat should definitely not be featured in the setup wizard. this is not competent software design and should never be resorted to

@TayouVR
Copy link
Member Author

TayouVR commented Jan 9, 2023

uhh, I don't like githubs feature to do this here, will redo that resolving conflicts locally and push in a second

@TayouVR
Copy link
Member Author

TayouVR commented Jan 9, 2023

@HyperSoop the cat being there doesn't hurt anyone and sure, I wouldn't have put it there, but you are free to remove it in a separate PR if you want. The function in question is this one: https://github.com/PrismLauncher/PrismLauncher/pull/249/files#diff-ee2722496429e9dee7a6ef8fd601be31996920cf7ef55ba3ec6ec8ac90bef2b6R64 (ThemeCustomizationWidget::showFeatures) read the comment above function to see what i had already tried without luck

@HyperSoop
Copy link
Contributor

I'm no programmer, i'm just giving my feedback. I cannot work on this, but this is clearly unacceptable to merge as is.

@HyperSoop
Copy link
Contributor

HyperSoop commented Jan 9, 2023

I'm not overexaggerating. Imagine being a new user and being told "hey, you really gotta choose the cat picture you want!" This is beyond stupid. This is not much by itself, but can snowball into more such horrible design choices accepted from PRs.

In fact, incompetent/incomplete/badly designed PRs have already been accepted, as an example take the old modpack updater.

@DioEgizio
Copy link
Member

It's not an horrible design choice? And if the cat wasn't there, the space would probably just not be used

@TayouVR
Copy link
Member Author

TayouVR commented Jan 9, 2023

yeah, the page is otherwise empty anyways...

@HyperSoop
Copy link
Contributor

You don't say "we need to use the space somehow" and add a meme atop your entire UI. this is literally it - plastering a huge meme onto it.

@DioEgizio
Copy link
Member

The cat isn't a meme though

@HyperSoop
Copy link
Contributor

HyperSoop commented Jan 9, 2023

It is an inside joke, this changes nothing. Still weird and unfitting.

@flowln
Copy link
Contributor

flowln commented Jan 9, 2023

I'm not overexaggerating. Imagine being a new user and being told "hey, you really gotta choose the cat picture you want!" This is beyond stupid. This is not much by itself, but can snowball into more such horrible design choices accepted from PRs.

In fact, incompetent/incomplete/badly designed PRs have already been accepted, as an example take the old modpack updater.

You are saying a page on a wizard someone sees once in the entire time they use the launcher is badly designed because we're using the extra space that would otherwise be empty to show a customization option in the customization page. Look at yourself.

And how about you stop saying shit about other people's free work? Time and time again you come here to shit on this project's choices without understanding a single thing about what is being done and why. You're being the incompetent here.

@TayouVR
Copy link
Member Author

TayouVR commented Jan 9, 2023

the cat is not a meme or joke. It has been part of MultiMC for like a decade. Exposing the option to set the cat here is fine. As I said before I didn't intend to originally, but theres nothing wrong with selecting it in the wizard.

@TayouVR
Copy link
Member Author

TayouVR commented Jan 9, 2023

on another note, here are some ugly hints...
image

@HyperSoop
Copy link
Contributor

HyperSoop commented Jan 9, 2023

You are saying a page on a wizard someone sees once in the entire time they use the launcher is badly designed because we're using the extra space that would otherwise be empty to show a customization option in the customization page. Look at yourself.

And how about you stop saying shit about other people's free work? Time and time again you come here to shit on this project's choices without understanding a single thing about what is being done and why. You're being the incompetent here.

Respecting other people's free work is no excuse to add bad changes conflicting with the going design philosophy and harming the UX.

The cat is not a highlightable feature of the launcher, not a feature that would sell it to people, and not one that you should recommend to new users right off. Look at yourself, exit the echo chamber you're in, and look at how stupid the whole situation is.

@TayouVR
Copy link
Member Author

TayouVR commented Jan 9, 2023

You are being extremely disrespectful and hurtful. As I originally said I agree, and didn't want to add the cat there myself either, but you did not provide constructive criticism, but instead harass and insult contributors for their free work & time. I will stop responding to you now, this is not worth my time.

@flowln
Copy link
Contributor

flowln commented Jan 9, 2023

Respecting other people's free work is no excuse to add bad changes conflicting with the going design philosophy and harming the UX.

The cat is not a highlightable feature of the launcher, not a feature that would sell it to people, and not one that you should recommend to new users right off. Look at yourself, exit the echo chamber you're in, and look at how stupid the whole situation is.

Respecting other people's free work means not bullshitting on people's work because the feature that works but could have improvements is an "incompetent/incomplete/badly designed PR" as you so like to say. And even more, a feature that only got released like it was because the whole project almost exploded because of other stuff. And even then, it's a one dialog thing that you can simply ignore, and you keep talking about it like it was hell on earth. Come on.

The cat is important enough that a sizeable amount of the community likes it and uses it. No one cares whether the launcher has a cat or not, besides you it seems. If they don't like the cat, they can just move on with their life without the cat. And yet you keep talking about it as if adding it to the customization wizard was a mortal sin. Maybe you should look at how stupid the whole situation is.

launcher/ui/widgets/ThemeCustomizationWidget.cpp Outdated Show resolved Hide resolved
launcher/ui/widgets/ThemeCustomizationWidget.h Outdated Show resolved Hide resolved
launcher/ui/setupwizard/ThemeWizardPage.ui Outdated Show resolved Hide resolved
@TayouVR TayouVR force-pushed the theme-selector-first-time-wizard branch from def3c60 to 77add06 Compare January 10, 2023 15:05
Tayou and others added 6 commits January 10, 2023 16:06
Signed-off-by: Tayou <tayou@gmx.net>
Signed-off-by: Tayou <tayou@gmx.net>
Signed-off-by: Tayou <tayou@gmx.net>
Co-authored-by: Sefa Eyeoglu <contact@scrumplex.net>
Signed-off-by: Tayou <tayou@gmx.net>
damn you visual studio for creating CRLF files everywhere...
Signed-off-by: Tayou <tayou@gmx.net>
Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
@TayouVR TayouVR force-pushed the theme-selector-first-time-wizard branch from 77add06 to 0db94b7 Compare January 10, 2023 15:07
Signed-off-by: Tayou <tayou@gmx.net>
@TayouVR TayouVR force-pushed the theme-selector-first-time-wizard branch from 0db94b7 to 668b19d Compare January 10, 2023 15:10
Copy link
Contributor

@flowln flowln left a comment

Choose a reason for hiding this comment

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

seems to work fine for me, thank you!

@flowln flowln added this to the 7.0 milestone Jan 10, 2023
@flowln flowln merged commit 96d5438 into PrismLauncher:develop Jan 10, 2023
Scrumplex added a commit to Scrumplex/PrismLauncher that referenced this pull request Apr 8, 2023
Closes PrismLauncher#490

Regression introduced by PrismLauncher#249

Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
pull bot pushed a commit to AtaraxiaSjel/PrismLauncher that referenced this pull request Apr 8, 2023
Closes PrismLauncher#490

Regression introduced by PrismLauncher#249

Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants