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

New hooks in files loading #34

Merged
merged 4 commits into from Mar 5, 2016
Merged

Conversation

@Henrybk
Copy link
Contributor

Henrybk commented Mar 4, 2016

  • Code review
  • QA testing

This is a request to add 3 hooks to Settings::loadByHandle, the purpose
of this request is to make it possible for plugins to change the filepath
and parsing method of any file just before it's loading. Be aware that
hook names depend on the loading file.

This is a request to add 3 hooks to Settings::loadByHandle, the purpose
of this request is to make is possible for plugins change the filepath
and parsing method of any file just before it's loading. Be aware that
hook names depend on the loading file.
@itsrachelfish

This comment has been minimized.

Copy link
Member

itsrachelfish commented Mar 4, 2016

Do you have an example plugin using these hooks that we can use for testing?

@Henrybk

This comment has been minimized.

Copy link
Contributor Author

Henrybk commented Mar 4, 2016

Sure, here is my current use of this Pull request code: http://pastebin.com/84X3rjny


#hooks of type 'pre_load_' make it possible to change the filename before openkore searches for it (this filename must contain file name and file path)
my %pre_load = (internalFilename => $internalFilename, filename => \$filename);
Plugins::callHook('pre_load_'.$internalFilename, \%pre_load);

This comment has been minimized.

Copy link
@allanon

allanon Mar 4, 2016

Member

I'm on a crusade against deeply nested logic. :)

How about:

my $pre_load = {internalFilename => $internalFilename, filename => $filename};
Plugins::callHook('pre_load_'.$internalFilename, $pre_load);
if ($pre_load->{return}) {
    $filename = $pre_load->{filename};
} elsif ($object->{autoSearch} && $object->{type} == CONTROL_FILE_TYPE) {
    $filename = _findFileFromFolders($object->{name}, \@controlFolders);
} elsif ($object->{autoSearch}) {
    $filename = _findFileFromFolders($object->{name}, \@tablesFolders);
} else {
    $filename = $object->{name};
}

Also, should we pass the $object->{type} as well, so plugins can make the same sort of decisions that this code does?

This comment has been minimized.

Copy link
@allanon

allanon Mar 4, 2016

Member

Oh right, the hook is on a per-file basis so presumably the receiver already knows the object type.

This comment has been minimized.

Copy link
@Henrybk

Henrybk Mar 4, 2016

Author Contributor

$filename = $pre_load->{filename}; is not necessary since in my code $pre_load{'filename'} holds a reference to '$filename' so it can be changed inside the plugin itself without any extra source code. About the nested logic, i guess since we are at it there could be no bad at doing some cleaning.

This comment has been minimized.

Copy link
@Henrybk

Henrybk Mar 4, 2016

Author Contributor

Also it's a good time to add the proper verification for 'TABLE_FILE_TYPE' instead of assuming everything that isn't a 'CONTROL_FILE_TYPE' is a 'TABLE_FILE_TYPE', this adds support for new file types in the future.

if ($object->{type} == CONTROL_FILE_TYPE) {
$filename = _findFileFromFolders($object->{name}, \@controlFolders);
my $internalFilename;
if ($object->{'autoSearch'}) {

This comment has been minimized.

Copy link
@allanon

allanon Mar 4, 2016

Member

Why does $internalFilename depend on $object->{autoSearch}? I don't know the context well enough to understand why this isn't just my $internalFilename = $object->{internalName} || $object->{name};.

This comment has been minimized.

Copy link
@Henrybk

Henrybk Mar 4, 2016

Author Contributor

$internalFilename depends on $object->{autoSearch} because if it is '1' then $object->{name} holds the file name, if it is '0' then $object->{name} holds a file path and $object->{internalName} holds the file name. Even so your idea sure is better than just a bunch of ifs.

my $internalFilename = $object->{internalName} || $object->{name};

#hooks of type 'pre_load_' make it possible to change the filename before openkore searches for it (this filename must contain file name and file path)
my %pre_load = (internalFilename => $internalFilename, filename => \$filename);

This comment has been minimized.

Copy link
@allanon

allanon Mar 4, 2016

Member

I'm not a big fan of passing scalar refs around as in/out variables. Perl doesn't make them easy to work with; when using them, one has to use the clunky double sigil ${$pre_load->{filename}} to read and write them. While it takes an extra line here to extract the modified $filename, it becomes easier for any hook handlers to work with the data.

OpenKore does use this idiom elsewhere though. Whatever you prefer is fine. :)

@Henrybk

This comment has been minimized.

Copy link
Contributor Author

Henrybk commented Mar 5, 2016

Plugin example updated to work without the scalar refs: http://pastebin.com/84X3rjny

@Henrybk

This comment has been minimized.

Copy link
Contributor Author

Henrybk commented Mar 5, 2016

Perl script to convert current portals.txt into JSON format to test with the plugin I gave: http://pastebin.com/Vb3f3Nqa

snop222 added a commit that referenced this pull request Mar 5, 2016
@snop222 snop222 merged commit 7b5d376 into OpenKore:master Mar 5, 2016
@allanon

This comment has been minimized.

Copy link
Member

allanon commented Mar 5, 2016

Please don't merge without checking all the boxes. I'm assuming you tested it first, so QA should have been checked?

allanon pushed a commit that referenced this pull request Aug 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.