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, configdialog: Factor out GenTab class from ConfigDialog #75233

Closed
terryjreedy opened this issue Jul 26, 2017 · 9 comments
Closed

IDLE, configdialog: Factor out GenTab class from ConfigDialog #75233

terryjreedy opened this issue Jul 26, 2017 · 9 comments
Assignees
Labels
3.7 (EOL) end of life topic-IDLE type-feature A feature request or enhancement

Comments

@terryjreedy
Copy link
Member

BPO 31050
Nosy @terryjreedy, @csabella
PRs
  • bpo-31050: IDLE: Factor GenPage class from ConfigDialog #2952
  • [3.6] bpo-31050: IDLE: Factor GenPage class from ConfigDialog (GH-2952) #2955
  • Dependencies
  • bpo-31004: IDLE, configdialog: Factor out FontTab class from ConfigDialog
  • 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-30.23:07:56.389>
    created_at = <Date 2017-07-26.19:18:52.478>
    labels = ['expert-IDLE', 'type-feature', '3.7']
    title = 'IDLE, configdialog: Factor out GenTab class from ConfigDialog'
    updated_at = <Date 2017-07-30.23:07:56.388>
    user = 'https://github.com/terryjreedy'

    bugs.python.org fields:

    activity = <Date 2017-07-30.23:07:56.388>
    actor = 'terry.reedy'
    assignee = 'terry.reedy'
    closed = True
    closed_date = <Date 2017-07-30.23:07:56.389>
    closer = 'terry.reedy'
    components = ['IDLE']
    creation = <Date 2017-07-26.19:18:52.478>
    creator = 'terry.reedy'
    dependencies = ['31004']
    files = []
    hgrepos = []
    issue_num = 31050
    keywords = []
    message_count = 9.0
    messages = ['299259', '299492', '299518', '299519', '299521', '299523', '299528', '299529', '299530']
    nosy_count = 2.0
    nosy_names = ['terry.reedy', 'cheryl.sabella']
    pr_nums = ['2952', '2955']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue31050'
    versions = ['Python 3.6', 'Python 3.7']

    @terryjreedy
    Copy link
    Member Author

    Followup to 31003, tests, and 30853, tracers, similar to 31004, FontTab.

    After creating new class, we can change names without worry about clashing with names elsewhere in ConfigDialog.

    @terryjreedy terryjreedy self-assigned this Jul 26, 2017
    @terryjreedy terryjreedy added the type-feature A feature request or enhancement label Jul 26, 2017
    @terryjreedy
    Copy link
    Member Author

    Now that we know what we are doing, we can simplify the steps. These assume that bpo-21004, PR2905, which prepares create_widgets and fixes the GeneralTest that was broken by Notebook, has been merged.

    For configdialog:

    • copy general block after FontPage;
    • add 'class GenPage(Frame):' and def __init__ modeled on FontPage.__init__, but no highlight parameter;
    • replace 'frame = dialog.tabpages...' at top of create_page_general with 'frame = self';
    • comment out old code;
    • in create_widgets change 'self.create_page_general' to 'GenPage(note)'.

    For test_configdialog:

    • change 'GeneralTest' to 'GenPageTest
    • change setUpClass similarly as in FontPageTest;
    • change test functions similarly as in FontPageTest and otherwise as needed to keep tests passing.

    @csabella
    Copy link
    Contributor

    Made all the changes without any issue.

    One thing - I noticed that the var_changed_autosave, etc for the General tab were back, even though they aren't used by VarTrace. I know I had deleted them, so I'm not sure how they came back. Unless you re-added them? That's what happened to me yesterday -- changes I knew I had made were back again. Anyway, if you want them back, sorry about deleting them again. I just figured it was something that happened on my end.

    Also, I snuck in a change to the Notebook. I know it should be in its own PR. If you like it, I can leave it here or make a new PR. :-)

    @terryjreedy
    Copy link
    Member Author

    Above, I left out the last step, "* remove old general block".

    The old configdialog had an unwritten and undocumented template and rules for pages on the dialog. It went something like the following.
    # def create_widgets():

    • Pick names for the pages. They are both tab labels and page ids.
    • Send names to tabbed_pages.
    • Call create_page_name functions.
      # def load_configs():
    • Call each load_name_config.
      # def create_page_name(): for each name.
    • Extract frame from tabbed_pages using name.
    • Define tk vars.
    • Create subframes and widgets
    • Pack widgets
      # def var_changed_var_name(), for each tk var
    • Perhaps send change to changes dict.
      # def other_method():, for each name, semi-sorted
      # def load_name_config() for each name.
      # Tack on extensions mostly ignoring the above.

    What I said in the message above is that we have redefined the template well enough to follow it. We have a mostly abstract template.

    class NamePage(Frame):
        def __init__(self, master):
            super().__init__(master)
            self.create_page_name()
            self.load_name_cfg()
    ...

    Note that the parameter is 'master', not 'parent', as is standard for widgets. It is also not saved, as it is not needed. TkVars for the page can use self as master.

    I considered making that an actual base class. But there is little common code; it does not quite fit FontPage; and would require uniform 'create_page' and 'load_cfg' names. The more specific names are easier to find with ^F ;-).

    I do want to add a comment block above FontPage giving the design. I also want to change FontPage to use self as master for tk vars and other widgets.

    Lets freeze the General page block until this is done.

    @terryjreedy
    Copy link
    Member Author

    I did not refresh this before writing the above. It still applies, including deleting the commented out code once tests pass. See review.

    Whoops: I forgot that using make_default means that we can (and should) delete the replaced var_changed defs. I added the one for space_num back because I mistakenly thought it was needed to make the test pass.

    I will open a new issue to add a comment documenting the new TabPage(Frame) class design. The new rules will include using default var_changes when possible. Patch will include adjusting FontTab to follow the new rules.

    @terryjreedy
    Copy link
    Member Author

    I intentionally left using non-default options other than traversal for a new issue or issues, after we finish ttk replacement. I imagine there will be discussions about choices, and I will invite other participants. I would like to direct visual design discussions, as opposed to internal implementation discussion, to idle_dev.

    @terryjreedy
    Copy link
    Member Author

    New changeset e8eb17b by Terry Jan Reedy (csabella) in branch 'master':
    bpo-31050: IDLE: Factor GenPage class from ConfigDialog (bpo-2952)
    e8eb17b

    @terryjreedy
    Copy link
    Member Author

    New changeset 8c4e5be by Terry Jan Reedy in branch '3.6':
    [3.6] bpo-31050: IDLE: Factor GenPage class from ConfigDialog (GH-2952) (bpo-2955)
    8c4e5be

    @terryjreedy
    Copy link
    Member Author

    Much easier the 2nd time ;-)

    @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 (EOL) end of life topic-IDLE type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants