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

IDLE: Document, fix, and complete configdialog font tests #75176

Closed
terryjreedy opened this issue Jul 22, 2017 · 14 comments
Closed

IDLE: Document, fix, and complete configdialog font tests #75176

terryjreedy opened this issue Jul 22, 2017 · 14 comments
Assignees
Labels
3.7 only security fixes topic-IDLE type-feature A feature request or enhancement

Comments

@terryjreedy
Copy link
Member

BPO 30993
Nosy @terryjreedy, @ned-deily, @mlouielu, @csabella
PRs
  • bpo-30981: IDLE: Add more configdialog font page tests. #2805
  • bpo-30993: IDLE - Improve configdialog font page and tests.  #2818
  • [3.6] bpo-30993: IDLE - Improve configdialog font page and tests. (G… #2826
  • bpo-30993: IDLE - Improve configdialog font page and tests. #2831
  • [3.6] bpo-30993: IDLE - Improve configdialog font page and tests. (GH… #2834
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/terryjreedy'
    closed_at = <Date 2017-07-24.06:51:31.735>
    created_at = <Date 2017-07-22.19:07:43.732>
    labels = ['expert-IDLE', 'type-feature', '3.7']
    title = 'IDLE: Document, fix, and complete configdialog font tests'
    updated_at = <Date 2017-07-24.06:51:31.734>
    user = 'https://github.com/terryjreedy'

    bugs.python.org fields:

    activity = <Date 2017-07-24.06:51:31.734>
    actor = 'terry.reedy'
    assignee = 'terry.reedy'
    closed = True
    closed_date = <Date 2017-07-24.06:51:31.735>
    closer = 'terry.reedy'
    components = ['IDLE']
    creation = <Date 2017-07-22.19:07:43.732>
    creator = 'terry.reedy'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30993
    keywords = []
    message_count = 14.0
    messages = ['298862', '298878', '298882', '298884', '298888', '298902', '298907', '298908', '298909', '298913', '298920', '298926', '298928', '298930']
    nosy_count = 4.0
    nosy_names = ['terry.reedy', 'ned.deily', 'louielu', 'cheryl.sabella']
    pr_nums = ['2805', '2818', '2826', '2831', '2834']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue30993'
    versions = ['Python 3.6', 'Python 3.7']

    @terryjreedy
    Copy link
    Member Author

    This follows bpo-30981 and the comments on PR 2805 after the close notice.

    • Causal chains in a directed acyclic graph link user actions to provisional entries in changes and and changes in the example displays. Explain these better in the docstring. Each font test function tests pieces of the graph.
      *test_font_set should run even if not run first.

    @terryjreedy terryjreedy added the 3.7 only security fixes label Jul 22, 2017
    @terryjreedy terryjreedy self-assigned this Jul 22, 2017
    @terryjreedy terryjreedy added topic-IDLE type-feature A feature request or enhancement labels Jul 22, 2017
    @terryjreedy
    Copy link
    Member Author

    Patch ready for test on linux and review.

    @csabella
    Copy link
    Contributor

    Works on linux. I just had one question on github about a test for sizelist and moving the initialization of the Tk variables into setUp.

    I also deleted a second comment that was just a wrong comment, but it's led me to another question.

    For test_indent_scale, you have the widget test and the changes test in the same place. I know you mentioned that you had thought about this before, but now that you have the widget tests for the font name, font bold, (and soon) font size, would there be any reason to split the test_font_set tests into the same place where each of the widgets is tested? That way, all the pieces of the graph flow would be in one place for each widget/Tk Variable/trace function.

    @terryjreedy
    Copy link
    Member Author

    I am thinking that the tab title should be 'Font/Indent' rather than 'Font/Tabs' to avoid the confusion between the simulated file-folder tab that we click on and the tab key that indents. Opinions?

    But maybe the indent widget will be moved, eliminating the issue, or maybe more will be added, as part of converting 'extensions' to features, requiring a different rename. 'Reformat paragraph, also affect text layout.

    @terryjreedy
    Copy link
    Member Author

    Yes, test_font_set could be split up. But then the setup would have to be repeated for each, and it would be harder to verify that each .set() call was being tested the same way.

    The event graph for indent_scale is a simple linear chain, and the simple liner test mirrors that.

    The combined test of bold_toggle and set_samples is not what I wanted, but I prefer it to the alternative of depending on an undocumented internal detail of the tcl wrapper maker (_tkinter, I suspect).

    @csabella
    Copy link
    Contributor

    +1 on the Font/Indent name. And I agree that the indent widget would make more sense on the General tab, or another name, like Editor/Shell Preferences.

    Another thought I had was Fonts should maybe be part of a Theme. At the theme level (not at the tag level), define font name/font size and then have font bold at the tag level. Spyder has that, so the function names (IDLE tag of definition) are bolded and the rest aren't. It's subtle, but I find it pleasing to work with because function/class/method names stand out. That would be after everything else though.

    Another thought I had with the cycle in fonts: A wdiget is clicked on, which calls the tkinter command callback. But, the Tk variable is also updated which calls the trace function. Those are the two paths. What if the trace function called set_samples? Then font_bold and font_size widgets wouldn't need the command argument. I think opt_menu_highlight_target does that now because the command is commented out.

    instead of:
    click bold-toggle ~> set font_bold -> var_changed_font
    |
    \---> call set_sample

    this:
    click bold_toggle -> set font_bold -> var_changed_font -> set_sample

    @terryjreedy
    Copy link
    Member Author

    New changeset 07ba305 by Terry Jan Reedy in branch 'master':
    bpo-30993: IDLE - Improve configdialog font page and tests. (bpo-2818)
    07ba305

    @terryjreedy
    Copy link
    Member Author

    A big issue with changing to tagging individual elements is back compatibility. Besides which, if the font is not bold, I cannot imaging bolding anything other than the definition names. This also seems to venture beyond 'keep IDLE simple for beginners'. How about a new option to just bold definition names?

    Factoring out the call to set_samples to one place is a great idea that simplifies code and tests. I did it and the htest dialog worked just fine. The tests also passed as were, but need reworking. I am doing that now. The result will be one FontTest class that tests that each widget sets its var, tests that var setting adds changes and calls set_samples, and tests set_samples.

    @terryjreedy
    Copy link
    Member Author

    New changeset 5aa3bf0 by Terry Jan Reedy in branch '3.6':
    [3.6] bpo-30993: IDLE - Improve configdialog font page and tests. (GH-2818) (bpo-2826)
    5aa3bf0

    @terryjreedy
    Copy link
    Member Author

    2nd and presumably last PR for this issue: please verify on linux.

    @terryjreedy terryjreedy changed the title IDLE: Document and fix configdialog font tests. IDLE: Document, fix, and complete configdialog font tests Jul 23, 2017
    @csabella
    Copy link
    Contributor

    The tests pass on linux and the htest looks good. Thank you for making all these changes. It seems much clearer now, but I do want to spend more time reading the code to understand it. Looks like you grouped all the font-related functions, so they are ready to become a Class? The only part I couldn't figure out is how to update highlight_sample since it wouldn't be in the class.

    With the backwards compatibility, I thought maybe you have an idea about how to make the changes compatible because of the issue that moves the extensions. I can see where that would be a problem as far as making changes is concerned.

    Maybe I'll look at the creating tests for one of the other tabs next?

    @terryjreedy
    Copy link
    Member Author

    New changeset 77e97ca by Terry Jan Reedy in branch 'master':
    bpo-30993: IDLE - Improve configdialog font page and tests. (bpo-2831)
    77e97ca

    @terryjreedy
    Copy link
    Member Author

    Ned, in a comment on PR2826, Louie reported "The patch unittest work on Linux, but on MacOS, I get test.support.ResourceDeined: cannot run without OS X gui process, it is wierd, I'm inside the GUI mode.

    Also, I'm not sure if this only on MacOS (is font problem or what), when checking bold box, some font won't render to bold (I've check that this may not relative to patch, in older commit have this problem, too)."

    Have you run gui tests, and in particular, IDLE tests on OSX lately? Is the OSX code in test.support for GUI required still working?

    @terryjreedy
    Copy link
    Member Author

    New changeset 1daeb25 by Terry Jan Reedy in branch '3.6':
    [3.6] bpo-30993: IDLE - Improve configdialog font page and tests. (GH-2831) (bpo-2834)
    1daeb25

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 only security fixes topic-IDLE type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants