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 an unnecessary wx translation underscore macro from PHackSettings. #179

Merged
merged 1 commit into from Mar 23, 2014

Conversation

lioncash
Copy link
Member

Also removed an unnecessary string -> wx string conversion.

…ngs.

Also removed an unnecessary string -> wx string conversion.
@neobrain
Copy link
Member

Uhh.. wait, is it actually all that unnecessary? I was thinking this was an actual ini value when I said it was redundant on IRC, but now that I look at the context it does seem okay, unless I'm missing something.

@delroth
Copy link
Member

delroth commented Mar 16, 2014

+1. OTOH, I'm not sure why we allow editing PHacks via the GUI. Or why we have PHacks at all, but that's another problem.

Sonicadvance1 added a commit that referenced this pull request Mar 23, 2014
Remove an unnecessary wx translation underscore macro from PHackSettings.
@Sonicadvance1 Sonicadvance1 merged commit 4c1cb65 into dolphin-emu:master Mar 23, 2014
@delroth
Copy link
Member

delroth commented Mar 23, 2014

Why was this merged? Can you confirm that translation was redundant? This is not a value that's going in an INI file, and nobody LGTM'd the change.

@Parlane
Copy link
Member

Parlane commented Mar 23, 2014

What is the difference between +1 and LGTM ?

@delroth
Copy link
Member

delroth commented Mar 23, 2014

The difference is that +1 is ambiguous when replying to a comment. I was +1-ing @neobrain's comment.

@Parlane
Copy link
Member

Parlane commented Mar 23, 2014

Interesting, looked unrelated to neobrains.

@Sonicadvance1
Copy link
Contributor

mmmm. I misread your comment.
This UI should really just die anyway.

@neobrain
Copy link
Member

You shouldn't merge things in the first place just because @delroth says "+1" when I already (effectively) said "-1". Instead, should have waited for a reply from @lioncash to clear up remaining discussions which cover critical criteria for merging. So.. yeah, it was pretty much irrelevant what @delroth was referring to.

@lioncash
Copy link
Member Author

Yeah, after further looking, this shouldn't have been merged, as that was the text shown for when the user want to specify a custom projection hack (although it should have plainly been "Custom").

However, in the discussion and sometimes in the IRC, it's said that the Projection Hack UI shouldn't even exist. Do you want me to remove the UI in its entirety, or just leave it as is?

@delroth
Copy link
Member

delroth commented Mar 23, 2014

I think it would make sense to remove this UI. The end goal is to have PHacks completely go away. Where they are used as enhancements, they should be replaced with cheat codes. Where they are used to work around bugs, the bugs should be fixed.

/me hates PHacks since they actually caused issues in the past, in SoAL for example.

@lioncash lioncash deleted the phack-cleanup branch January 25, 2017 01:15
Joern-P pushed a commit to Joern-P/dolphin that referenced this pull request Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants