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

Remove DolphinWX #6819

Merged
merged 1 commit into from
Jun 29, 2018
Merged

Remove DolphinWX #6819

merged 1 commit into from
Jun 29, 2018

Conversation

spycrab
Copy link
Contributor

@spycrab spycrab commented May 11, 2018

Just testing the waters and seeing what issues will occur.

This obviously should only get merged once most of the Qt regressions and (minor) missing features are implemented

@spycrab spycrab added WIP / do not merge Work in progress (do not merge) RFC Request for comments labels May 11, 2018
@spycrab spycrab force-pushed the delete_wx branch 2 times, most recently from 08a30ec to 1fd497e Compare May 11, 2018 18:59
@Helios747
Copy link
Contributor

I'm fine with this once the more severe issues logged on the tracker are resolved.

@ghost
Copy link

ghost commented May 11, 2018

  • There's some in Base.props , and the WXWOverrides.props looks less sure.
  • There is a WX define in GekkoDisassembler.cpp - Line 1019 - EDIT: less sure because a lot of "blablawx" stuff around there.
  • GLInterfaceBase.h - Line 60
  • Common.h - Line 65
  • MsgHandler.cpp - Line 27 and 33
  • BootManager.h - Line 12
  • Gekko.h - Line 274 not sure
  • RenderBase.h - Line 174 not sure

I searched in-browser and did not found any on the Files Changed page, I scrolled all the page to make sure the page loaded all the content and it seems like it did. However after a while the tab crashed in Opera with an "out of memory" error a few times but it was generally fine when I avoided pressing any buttons, search worked fine.
VSProps folder nor .props seem to be gitignored, and my VS did not modify these files, based on that I thought it's okay to raise this, I also tried to learn if there's some kind of auto-updating that will remove these lines once the main project is removed but found no info on that, but it probably wouldn't hurt if these were removed right here.

@spycrab
Copy link
Contributor Author

spycrab commented May 11, 2018

Most of the stuff you mentioned should be removed later on.

There's some in Base.props , and the WXWOverrides.props looks less sure.

That should probably be removed with this PR though.

@lioncash
Copy link
Member

GekkoDisassembler.cpp - Line 1019
Gekko.h - Line 274

These two are unrelated to wxWidgets

@Ebola16
Copy link
Member

Ebola16 commented May 13, 2018

Dolphin.exe is being compiled in VS but the debugger is using DolphinWx.exe by default. Do you know how to change this? I've also made a Dolphin forum post. This needs to be resolved before getting rid of DolphinWX.

@fancythedeveloper
Copy link

Make sure to add "Path to Virtual SD Card" from WX to QT before merging

@spycrab
Copy link
Contributor Author

spycrab commented May 13, 2018

Make sure to add "Path to Virtual SD Card" from WX to QT before merging

That was added over two weeks ago...

@sepalani
Copy link
Contributor

Regardless, IMHO, I don't think this PR should be blindly merged.

ATM, there are too many annoyances/flaws that prevent some implemented features to be used at their fullest on Qt. That is/was true, especially for the debugger part of Dolphin. Even if WxWidgets works but Qt has nice additions over it, it shouldn't be used as an excuse to merge this PR too early. I think that's also why @spycrab said that it won't be merged before Qt regressions are fixed and missing features implemented.

Dolphin still has parts where WxWidgets is uglier but works better than its Qt counterpart while code-wise it's probably cleaner on Qt but still needs to be polished.

@spycrab
Copy link
Contributor Author

spycrab commented May 29, 2018

We're reaching a state where I'd be more confident in this being mergable.
Any opinions? Especially if there are certain things we should wait for?

cc: @JMC47 @MayImilae

@spycrab spycrab force-pushed the delete_wx branch 2 times, most recently from 3ca1315 to 3944806 Compare May 29, 2018 02:32
@spycrab
Copy link
Contributor Author

spycrab commented May 29, 2018

Dolphin.exe is being compiled in VS but the debugger is using DolphinWx.exe by default.

This PR makes it default to Qt now but keep in mind that your user preferences might still override this.

@spycrab spycrab removed the WIP / do not merge Work in progress (do not merge) label May 29, 2018
@spycrab spycrab changed the title [DO NOT MERGE] Remove DolphinWX Remove DolphinWX May 29, 2018
@ghost
Copy link

ghost commented May 29, 2018

My remaining tickets are few and low priority, I have nothing pending, seen nothing suspicious since. Reminder about base.props and WXWOverrides.props

@MayImilae
Copy link
Contributor

MayImilae commented May 29, 2018

My opinion is that we should not do remove WX yet. Whether technically ok now or not, it's just too soon. We need to give several months of leaving things be and letting users get used to Qt before we remove WX. So let WX rot a bit, let even stubborn users move over as new features are implemented only in Qt, and then we can put WX to bed.

Also removing apply this soon goes directly against what we've told users in the Progress Reports and the Qt as default article.

I absolutely do not agree with merging this before 2019. I'm ok with 2019, but in my opinion removing WX really should be a part of the release process for 6.0.

@JMC47
Copy link
Contributor

JMC47 commented May 29, 2018

Even when all issues are fixed, there's no rush to remove it.

@Ebola16
Copy link
Member

Ebola16 commented May 29, 2018

Dolphin.exe is being compiled in VS but the debugger is using DolphinWx.exe by default. Do you know how to change this? I've also made a Dolphin forum post. This needs to be resolved before getting rid of DolphinWX.

Should be fixed in #7019

@spycrab
Copy link
Contributor Author

spycrab commented May 29, 2018

Even when all issues are fixed, there's no rush to remove it.

Sure.

To clarifiy: I have no problem with waiting a few more months but

I absolutely do not agree with merging this before 2019.

Is just over the top. I think September of this year is a more realistic date that should allow for enough time to have everything sorted out more than thoroughly.

Also keep in mind that many changes in Core require updating wx as well and we all know what that entails...

@ghost
Copy link

ghost commented May 29, 2018

Just a thought: What about branching Qt improvement to a Wx-less one while the Qt momentum is still high and memory is fresh, I can only speak for myself but If I had a big pause in between I would take some time to get back into gears. And the stuff I mentioned in my GUI thread would be way easier without Wx, but none of that is a strong opinion, so no need to take it that seriously.

@Helios747
Copy link
Contributor

... Why

you gain nothing.

@ghost
Copy link

ghost commented May 29, 2018

I don't know if it's worth it or not, I was merely suggesting that to those who were planning it. AFAIK it was decided way long ago that first Qt would copy Wx design, only then changes would come and would only be on Qt from that point forward. I stopped bothering after that decision was made, except the stuff I had stashed prior to that, that's what I posted in the thread for anyone to take into account if they wish, that's it from me.

@Helios747
Copy link
Contributor

We don't need to branch with Wx entirely removed.......

We can just.... Ignore that Wx is there.

@MayImilae
Copy link
Contributor

MayImilae commented May 29, 2018

Is just over the top. I think September of this year is a more realistic date that should allow for enough time to have everything sorted out more than thoroughly.

My concern isn't about your ability to fix all of the Qt issues. I know that's going to happen pretty soon! What I'm worried about is how users react to the full removal of WX. We want them to like Qt! We want them to feel like this is all good and great for them! But a lot of users may have tried Qt immediately and switched back to avoid bugs, or just refused to change over outright. If we just yank WX away not even 6 months after Qt was made default, they'll feel like we've stolen it from them, and we're going to have protests. But if we let WX rot for a year and then remove it, most of the holdouts will have seen the features they are missing out on grow and grow and made the switch on their own, reducing the number of users who feel "stolen" from. Plus we'll have SO MANY positive features that users making the switch will gain, making it a much easier sell!

I just want to make sure Qt is liked when we finally remove WX, and as few people feel mad about it as possible. We don't want a "WX 4ever" fork.

Besides, it doesn't really hurt us to let WX rot a bit, but it could really help us sell Qt.

@spycrab
Copy link
Contributor Author

spycrab commented May 29, 2018

Besides, it doesn't really hurt us to let WX rot a bit, but it could really help us sell Qt.

Except when it does though. Just today I had to introduce a rather nasty hack into Qt because of Wx.

And there's also this (selfquote):

Also keep in mind that many changes in Core require updating wx as well and we all know what that entails...

Keeping Wx around definitely has a cost.

@spycrab
Copy link
Contributor Author

spycrab commented May 29, 2018

Just to be clear: I don't have any problems waiting a bit longer for users to get used to Qt and convincing as many as possible to switch voluntarily. But six months seem unreasonably long for that.

@Ebola16
Copy link
Member

Ebola16 commented Jun 3, 2018

Yeah, I guess there's no point in removing that. Nevermind.

@spycrab
Copy link
Contributor Author

spycrab commented Jun 21, 2018

Now that we have some improved statistics - some observations:

The whole 'voluntarily' thing doesn't really seem to work. The amount of users using wx is - and always has been since the default switch - stuck at 20%. There is nothing that indicates this will change at any point in the future.

So no matter when we switch there'll be 1/5 of all users that could potentially be upset about it.

cc: @MayImilae @JMC47

@MayImilae
Copy link
Contributor

MayImilae commented Jun 21, 2018

In all of our big removals, we've always given at least 6 months warning, but typically it's more like a year to 18 months. We've even waited 2 years in some cases. I know you are really itching to remove WX at this point, but we really just need to give users time.

Plus with more time we can hammer home that we're removing it over multiple Progress Reports to get the userbase as ready for this as possible, both informing anyone who isn't aware of it yet and making sure as many people as possible know that WX is really actually going away so they won't be surprised. And hopefully that will reduce that 20% figure even more, but if not, it will minimize it as much as we can for a reduced (not 18 months) time frame.

@shuffle2
Copy link
Contributor

Not sure what repeatedly publicizing that Dolphin's going to switch UI frameworks accomplishes (especially since it's already been done).
If Qt has feature parity and the new "dolphin.exe" is Qt instead of WX, what does it matter when exactly the change happens? Dolphin hasn't even had a stable release for ages at this point.
Just Do It.

@Dockolm
Copy link

Dockolm commented Jun 22, 2018

Then we should at least wait the next progress report and clearly announcing WX will be dropped sooner than expected, probably before the next PR.
Qt's UI being very close to WX, it shouldn't be such a big deal.

@orbea
Copy link
Contributor

orbea commented Jun 22, 2018

I agree with @MayImilae. Deprecated features should be given time, I personally would suggest making at least one more stable release with wx.

@tony971
Copy link

tony971 commented Jun 22, 2018

To be fair, this isn't your standard depreciated feature. As Qt hits feature parity, there won't be a use case in which Wx is more useful. Compare that to say, DX9, and you could easily name some instances in which some users would be impacted by its removal.

@MayImilae
Copy link
Contributor

MayImilae commented Jun 22, 2018

@orbea We have already stated that WX will be removed before the next stable release. Remember that the next Stable release is probably another year or two from now, and no one wants to keep it around that long!

@delroth
Copy link
Member

delroth commented Jun 22, 2018

20% of users is a lot. Do we know what's pushing these users to keep using WX? If not, have you considered putting some feedback feature in the WX UI? e.g. a butterbar above the game list that says "We want to get rid of this UI soon, please talk to us on this forum thread to tell us why you keep using it."

@MayImilae
Copy link
Contributor

MayImilae commented Jun 22, 2018

That's a great idea! If we add some some of WX warning about it being deprecated (akin to the old Qt in development warning) and then wait a few months, that would be a great approach.

@spycrab
Copy link
Contributor Author

spycrab commented Jun 22, 2018

Wx being around definitely hinders implementing new features in Qt. If we want to keep around longer it will obviously negatively impact its progress. Hope that's clear.

@JMC47
Copy link
Contributor

JMC47 commented Jun 22, 2018

I think it's pretty clear that a lot of big rewrites are blocked by Wx being around. I'm not so sure it's important to keep Wx around (when it's slowing down or even stopping Qt's development) when we still plan on making Qt better anyway.

The best way to get teh remaining 20% of users to complain is to remove Wx. That way we'll know what's wrong with Qt and be able to fix it. If we wait, we may not have the people around to do so.

@MayImilae
Copy link
Contributor

MayImilae commented Jun 22, 2018

We should not just suddenly pull the rug out from thousands of users. :/ Yes it may suck to keep WX around for several more months, but we have to do this properly and with great care. We want everyone to celebrate the removal of WX, and not despise it and make a bloody WX fork. And yes that could happen; we've seen it happen for far sillier things.

@spycrab
Copy link
Contributor Author

spycrab commented Jun 22, 2018

Yes it may suck to keep WX around for several more months

Several more months without new Qt features it is then.

@Bombastisch
Copy link

I am sorry to pop in, but I think I know the reason why there are these constant 20%.
These useres probably have a desktop icon linking to Wx since qt was still a bit buggy in the beginning. If the users then update manually the desktop icon still links to DolphinWX and some of the users might not even notice.

@sepalani
Copy link
Contributor

@spycrab I don't get why keeping an outdated UI slows down Qt progress while some/most of the new features are Qt only. Beside config changes and some other stuff I don't see why we can't leave it for the time being.

IMHO, as I already stated, the root problem isn't Wx itself but the way the code has been integrated to the UI and Qt is no exception to that. If you want, I can pinpoint the pros and cons of Qt vs the ones of Wx and show you that both are doing wrong and sometimes one is doing better than the other. However, in the end it's mostly a tie regarding which one is better (from someone using Dolphin and its debugger).

If there are blocking points, then, design-wise it's already bad and moving the code to Qt won't help this situation (copy-paste shenanigans). The real solution would be to rework the interface and the way it's used in the UI. That way it would make easier the UI creation job, especially if we plan to support other backends such as UWP or others at some point.

Regarding the remaining users, some might use Dolphin debugger and might want to stick with the same version to avoid savestate issues or regressions. That even makes more sense if they're using portable builds.

@JMC47
Copy link
Contributor

JMC47 commented Jun 23, 2018

A lot of hte config stuff we wanna do requires significant rewriting, and doing the work on both Wx and Qt jsut isn't reasonable when we're planning on dropping Qt. I've also noticed that a couple of developers are waiting for Wx to drop before working on features that interact with the GUI.

If people are using old builds for Wx, I don't see why keeping Wx around affects that. It's not like old savestates and whatnot are going to magically start working in new builds.

@delroth
Copy link
Member

delroth commented Jun 23, 2018 via email

@spycrab
Copy link
Contributor Author

spycrab commented Jun 23, 2018

Do what you want. I don't care anymore. ¯\_(ツ)_/¯

@newblar
Copy link

newblar commented Jun 28, 2018

I have noticed that on Windows 10 using that search on the taskbar if you type in Dolp best match shows Dolphin.exe but if you type Dolph Windows then shows DolphinWX as best match. Could be why some people keep using WX.

@spycrab spycrab merged commit e22c533 into dolphin-emu:master Jun 29, 2018
@spycrab spycrab deleted the delete_wx branch June 29, 2018 22:12

if(USE_DISCORD_PRESENCE)
message(STATUS "Using static DiscordRPC from Externals")
add_subdirectory(Externals/discord-rpc)
endif()
endif()

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request for comments