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

Position and signed enums #244

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Position and signed enums #244

wants to merge 1 commit into from

Conversation

mrJean1
Copy link
Collaborator

@mrJean1 mrJean1 commented Jan 4, 2023

This is an attempt to correct (a) the Position enum values, see Parser.FIXME_enums and (b) generate signed and unsigned enums, see PythonGenerator.generate_enums from issue #243.

This is an attempt to correct the Position enum values, see Parser.FIXME_enums and generate signed and unsigned enums, see PythonGenerator.generate_enums.
Copy link
Owner

@oaubert oaubert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. I have some naming issues:

  • the "FIXME" is by convention a marker for code needing work. I cannot quite understand from the current code if it used in this way here (indicating that we should definitely find a better fix). In all cases, I would rather name the variable as hardfixed_enums for instances, and possibly add a # FIXME comment to indicate there is more work needed.
  • the Enum/Enim is nice, but takes a while to understand. Would it not be better to use explicitly UnsignedEnum / SignedEnum?

One last thing: the hardcoded value for libvlc_position_top seems wrong to me. AFAIK, it should be 3, not 4 ? And 6 for libvlc_position_bottom, no?

@oaubert
Copy link
Owner

oaubert commented Jan 4, 2023

For my last comment (about enum values) - my remembering of the C spec is correct, running the following code

#include <stdio.h>
#include <stdlib.h>

#include <vlc/vlc.h>

int main(int argc, char **argv)
{
    printf("libvlc_position_disable = %d\n",      libvlc_position_disable);
    printf("libvlc_position_center = %d\n",       libvlc_position_center);
    printf("libvlc_position_left = %d\n",         libvlc_position_left);
    printf("libvlc_position_right = %d\n",        libvlc_position_right);
    printf("libvlc_position_top = %d\n",          libvlc_position_top);
    printf("libvlc_position_top_left = %d\n",     libvlc_position_top_left);
    printf("libvlc_position_top_right = %d\n",    libvlc_position_top_right);
    printf("libvlc_position_bottom = %d\n",       libvlc_position_bottom);
    printf("libvlc_position_bottom_left = %d\n",  libvlc_position_bottom_left);
    printf("libvlc_position_bottom_right = %d\n", libvlc_position_bottom_right);
      return 0;
}

indeed returns

libvlc_position_disable = -1
libvlc_position_center = 0
libvlc_position_left = 1
libvlc_position_right = 2
libvlc_position_top = 3
libvlc_position_top_left = 4
libvlc_position_top_right = 5
libvlc_position_bottom = 6
libvlc_position_bottom_left = 7
libvlc_position_bottom_right = 8

@mrJean1
Copy link
Collaborator Author

mrJean1 commented Jan 4, 2023

Please, do use better names instead of the current, acronymic ones.

The top value is 4 in the screenshot in issue #243, the documentation also shows top=4. Using 4 in cocoavlc.py puts the marquee at the top in the middle, 3 puts the marquee on the center left (and not at the top in the middle), and 7 on the top left (and not at the bottom left).

@mrJean1
Copy link
Collaborator Author

mrJean1 commented Jan 5, 2023

For your consideration, attached is the generator.py and generated vlc.py (from VLC 3.0.18 on macOS) with the new names _EnumSigned, _EnumUnsigned, enum2EnumSigned and hardfixed_enums with a preceeding # FIXME ... comment.

Also, all 9 Position values have been tested with the video_set_marquee_int(VideoMarqueeOption.Position, ...) option and all 9 appear in the proper location with the Position name.

The libvlc_position_bottom_right case was misspelled in the previous generate.py file.

generate1.21.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants