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

Winery engines ordering #33

Open
vitor251093 opened this issue Jun 30, 2018 · 15 comments
Open

Winery engines ordering #33

vitor251093 opened this issue Jun 30, 2018 · 15 comments
Labels
enhancement New feature or request

Comments

@vitor251093
Copy link
Owner

Based in the Discord discussion, I've added the ordering system that is present today in PK to Winery :)

Like we discussed, I've added the change to a branch instead of merging it directly.

@vitor251093 vitor251093 added the enhancement New feature or request label Jun 30, 2018
@Gcenx
Copy link
Collaborator

Gcenx commented Jun 30, 2018

Nice I will add that into my current branches when I'm at my Mac.

Nice, I made a note in the pull-request my commit history is a complete mess and this new branch now it makes sense that I close that fix my commit history then do a new pull-request to the new branch.

It will also include structure changes to the Master Wrapper, since we dropped WineskinX11 I removed all the bash scripts that it needed etc, plus with that removal the needed dylibs shrunk by alot

@vitor251093
Copy link
Owner Author

Nice, I made a note in the pull-request my commit history is a complete mess and this new branch now it makes sense that I close that fix my commit history then do a new pull-request to the new branch.

Okidoki ;)

It will also include structure changes to the Master Wrapper, since we dropped WineskinX11 I removed all the bash scripts that it needed etc, plus with that removal the needed dylibs shrunk by alot

That's great news :D

@Gcenx
Copy link
Collaborator

Gcenx commented Jun 30, 2018

I just tested it and it says my engines are all corrupted, also shows file extensions within the name

Not too sure if it's an error with the ordering code or an issue on my end when I merged it into my devel branch

@vitor251093
Copy link
Owner Author

Sorry, it was a mistake of mine. Just fixed.

@Gcenx
Copy link
Collaborator

Gcenx commented Jun 30, 2018

Sorry, it was a mistake of mine. Just fixed.

That got it, now next question is what Branch do you want me to push my Squashed Development Branch too? Ugh never mind Development has conflicts....., I will remake the branch offline so it's upto-date fully and a pull-request wont cause any issues .

If you added README.md to .gitignore wouldn't that stop the merging of my edited README.md?

@Gcenx
Copy link
Collaborator

Gcenx commented Jul 7, 2018

@vitor251093 another thing that you should add is if XQuartz is not installed only show Mac Driver compatible engines, since we don't want people saying it does not work since they didn't read that they needed to install XQuartz

@vitor251093
Copy link
Owner Author

vitor251093 commented Jul 7, 2018

If you added README.md to .gitignore wouldn't that stop the merging of my edited README.md?

Uhn, that may work. I will try that.

@vitor251093 another thing that you should add is if XQuartz is not installed only show Mac Driver compatible engines, since we don't want people saying it does not work since they didn't read that they needed to install XQuartz

Certainly there must be a difference, but how about this: when someone tries to create a port with an engine that is only XQuartz compatible, and XQuartz isn't installed, we show the user a warning instead, with the link to the XQuartz's website.

What do you think?

@Gcenx
Copy link
Collaborator

Gcenx commented Jul 7, 2018

Certainly there must be a difference, but how about this: when someone tries to create a port with an engine that is only XQuartz compatible, and XQuartz isn't installed, we show the user a warning instead, with the link to the XQuartz's website.

That sounds like an option, I'm thinking with the whole don't list them idea since after Mojave we might not even have access to XQuartz anymore. So only showing compatible engines came more to my mind.

So how about a warning first time if XQuartz is not installed, if its not installed then just stop listing Engines that require it?

@vitor251093
Copy link
Owner Author

Still too harsh. People may don't have internet available in the first time.

If macOS 10.15/11.0 kills XQuartz, we can make Winery remove those items from the list specifically in macOS 10.15+/11.0+.

@Gcenx
Copy link
Collaborator

Gcenx commented Jul 8, 2018

That’s true I just didn’t want it to be annoying. But we still need to stop listing engines that can not be used. Since that will cause headaches.

@vitor251093
Copy link
Owner Author

Certainly, although we still need to show those engines, to show that they are still option, even if they have special requirements.

We could make engines that have requirements red, or orange, to differ them from the rest. If people try to install red/orange engines, the warning with their requirement is shown (like that the engine required XQuartz, in those cases).

However, we can also give the option to hide those engines, with a checkbox for example, and it can be enabled by default.

@Gcenx
Copy link
Collaborator

Gcenx commented Jul 8, 2018

I agree 100% with that way of handling it.
Red should be the color since most people should understand Red means stop/error/warning, and having a checkbox being enabled by default and warning message should solve any potential user error.

@vitor251093
Copy link
Owner Author

Alright, just did it :)

Here follow the images of the checkbox, enabled and disabled. Ignore the fact that I'm running Mojave's Dark mode.

screen shot 2018-07-12 at 22 17 59

screen shot 2018-07-12 at 22 18 02

@vitor251093
Copy link
Owner Author

vitor251093 commented Jul 13, 2018

Also, if you disable the box and XQuartz is not installed, you gonna receive a warning, telling you that you need XQuartz, and providing the download link.

I've committed it to that same branch, the Wineskin engines ordering, since it's still kinda related to it.

@Gcenx
Copy link
Collaborator

Gcenx commented Jul 13, 2018

Looks very good, I just won't dare to start using that as my daily driver for now.

I will pull this and 7za change into my offline branch to check everything is playing nice.
I already mostly have the #37 changes done for Wineskin.app I'm just doing some basic bug testing. And I might have found another fun bug for me to squash I will post an issue if it turns out to be a Wineskin issue.

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

No branches or pull requests

2 participants