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

JANITORIAL: Rename EVENT_RTL #2240

Merged
merged 6 commits into from May 12, 2020
Merged

JANITORIAL: Rename EVENT_RTL #2240

merged 6 commits into from May 12, 2020

Conversation

@Mataniko
Copy link
Contributor

Mataniko commented May 10, 2020

Now that ScummVM is adding RTL support, the EVENT_RTL should be disambigious that it is for returning to launcher.

Please review with whitespace diffing off, as the PR also eliminated trailing spaces in the modified files

@dreammaster
Copy link
Member

dreammaster commented May 11, 2020

Hmm.. I've been adding in EVENT_RTL in engines previously without ever properly thinking about it. Can someone tell me the rationale of having EVENT_RTL as a separate event? It seems like currently it's all too easy for an engine not to add it in addition to EVENT_QUIT, and then end up having trouble later on due to it's absence. For example, the director engine in events.cpp has a case for EVENT_QUIT to to set sc->_stopPlay = true. But not for EVENT_RTL as well.

Do engines really need to have separate events for one vs the other. Could we instead perhaps have a base engine flag "_returnToLauncher" that the default_events.cpp could set. I mean, that block already references g_engine, so if seems like it would be better to having to have engines deal with both events all the time.

@tsoliman
Copy link
Member

tsoliman commented May 11, 2020

Originally EVENT_RTL was supposed to come from as a user input (from a backend)

Our event system doesn't really have separation between Engine-originated and Backend-originated events.

IMHO, an engine shouldn't really have a concept of RTL because that's very much a ScummVM construct. At best an engine can EVENT_QUIT.

I tried to overhaul the event framework several times to achieve that separation (and have the delineating line be the EventMapper) but it is super intrusive (and honestly beyond my skills since it touches every engine and every backend).

@dreammaster
Copy link
Member

dreammaster commented May 11, 2020

Well, considering that some of the event types have their own data, could we perhaps simply add a "_returnToLauncher" flag to the event class instead? It would require minimal changes, and it would mean the engines would only need to check for EVENT_QUIT now instead of both.

@Mataniko
Copy link
Contributor Author

Mataniko commented May 11, 2020

I thought it was because each engine has to implement it in order to clean up properly and return to the launcher, otherwise you need to quit ScummVM.

Either way, this PR is to address any future disambiguation with RTL text

@dreammaster
Copy link
Member

dreammaster commented May 11, 2020

I guess you're right. This PR seems fine as is. Anything else can be discussed further in-channel in the future

@sev-
Copy link
Member

sev- commented May 11, 2020

Yes, the history of EVENT_RTL is that the engine authors have to implement a proper cleanup and re-entry to the engine. When engines are statically built, lack of both could be a problem.

Once an engine supports it, feature flag kSupportsRTL should be defined in this engine MetaEngine overrides. Otherwise, the "Return to Laucher" button is disabled in the GMM.

Particularly, Director engine still leaks a lot, thus, return to launcher is not supported. And I just realized that there is a leftover feature from skeleton engine in Director.

@sev-
Copy link
Member

sev- commented May 11, 2020

...and this leads me to the suggestion to rename the feature as well. @Mataniko could you extend this PR?

@Mataniko Mataniko force-pushed the Mataniko:master branch from eb3bbf8 to 8046d12 May 12, 2020
@Mataniko
Copy link
Contributor Author

Mataniko commented May 12, 2020

@sev- This should take care of all references to RTL

@sev-
Copy link
Member

sev- commented May 12, 2020

thanks!

@sev- sev- merged commit 1b9fc31 into scummvm:master May 12, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.