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: gracefully handle themes (or keysets, or ...) not present #69500

Closed
roseman mannequin opened this issue Oct 4, 2015 · 12 comments
Closed

IDLE: gracefully handle themes (or keysets, or ...) not present #69500

roseman mannequin opened this issue Oct 4, 2015 · 12 comments
Labels
topic-IDLE type-feature A feature request or enhancement

Comments

@roseman
Copy link
Mannequin

roseman mannequin commented Oct 4, 2015

BPO 25313
Nosy @terryjreedy, @kbkaiser, @serwy, @roseman
Files
  • write-new-defaults.patch
  • missingtheme.patch
  • @cfg_text.diff
  • 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 = None
    closed_at = <Date 2015-11-12.20:38:32.043>
    created_at = <Date 2015-10-04.18:23:48.311>
    labels = ['expert-IDLE', 'type-feature']
    title = 'IDLE: gracefully handle themes (or keysets, or ...) not present'
    updated_at = <Date 2015-11-12.20:38:32.042>
    user = 'https://github.com/roseman'

    bugs.python.org fields:

    activity = <Date 2015-11-12.20:38:32.042>
    actor = 'terry.reedy'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-11-12.20:38:32.043>
    closer = 'terry.reedy'
    components = ['IDLE']
    creation = <Date 2015-10-04.18:23:48.311>
    creator = 'markroseman'
    dependencies = []
    files = ['40674', '40693', '40939']
    hgrepos = []
    issue_num = 25313
    keywords = ['patch']
    message_count = 12.0
    messages = ['252278', '252282', '252283', '252292', '252301', '252370', '252372', '252441', '254043', '254044', '254553', '254556']
    nosy_count = 5.0
    nosy_names = ['terry.reedy', 'kbk', 'roger.serwy', 'markroseman', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue25313'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5', 'Python 3.6']

    @roseman
    Copy link
    Mannequin Author

    roseman mannequin commented Oct 4, 2015

    As a follow-on to bpo-24820, when a particular theme is selected in the configuration files, but that theme is not available, IDLE will print out a bunch of warning messages on console. That can occur for example when using a newer built-in theme in an older version before that theme was added, or if the user highlight config file was modified or removed.

    IDLE should be smart enough to recognize that and switch to a default theme, without dumping out console error messages. May alert user to that case with an appropriate error message, e.g. theme xyz not found, using default.

    The current workaround in bpo-24820 (when a newer theme is chosen in a new version, explain it may not work in older version but explain how it could be made to) is unfortunately necessary unless a more clever solution for this backwards compatibility issue can be found.

    Would apply to keysets, etc. in the same was as themes.

    @roseman roseman mannequin added topic-IDLE type-feature A feature request or enhancement labels Oct 4, 2015
    @roseman
    Copy link
    Mannequin Author

    roseman mannequin commented Oct 4, 2015

    Actually, I think we may be able to get away without the warning message when you select a 'new' theme, and still maintain backwards compatibility.

    For 'new' themes (i.e. IDLE Dark and any more builtins we add in the future), we write the theme out to the user configuration file, keeping the same name, and perhaps adding an extra field, i.e. 'builtin=1', so that we can recognize it in newer versions, along with a comment if possible.

    The existing code looks up theme names by checking if its in the default list, and if not, looks in the user list. For older versions without IDLE Dark, it will then find it in the user list.

    Besides writing out the theme to user config file, the only other change that would be needed in the code is to modify GetSectionList so that when it's looking in a user config file, it doesn't include sections that either have the builtin=1 from above, or just have the same name as things in the default file.

    @roseman
    Copy link
    Mannequin Author

    roseman mannequin commented Oct 4, 2015

    Patch write-new-defaults.patch attached so that we write 'newer' default themes to config-highlight.cfg if selected, and ignore them if we already have a default by that name.

    @terryjreedy
    Copy link
    Member

    There over-arching issue is that IDLE was not built to allow addition of new builtin themes. IDLE New is currently identical to IDLE Classic, so maybe that was intended to allow a revision of IDLE Classic (and I have an idea for that), but IDLE Dark is too drastically different to have added it as IDLE New. There are two sub-issues here.

    1. Theme loading: I experienced the multiple error messages to console behavior once, but only once. This is the 'or worse' in the new message. I could not find out why only once, or know that the same would be true on other machines. If possible, there should only be one message. Whether it goes to console or messagebox is a different issue, part of revisiting the configure error message system.

    The first fallback should be to existing and accessible builtins, namely IDLE Classic, rather than a default of no colors. This strikes me as a buggy design.

    1. New theme addition and selection: This has to take into account that we cannot patch existing releases and that all IDLEs used by one user use the same user configuration files. (There is an issue post that suggests maybe changing this, but I cannot find it now.) This is a minus in that we cannot currently restrict selection of IDLE Dark to new IDLEs (but see below). This is a plus in that IDLE Dark can be 'backported' by saving as a custom theme. We may always need some sort of message.

    I thought about automatically saving as a custom theme, but besides the presumed deadline for 3.4.4 (see below), there are potential problems. A) It is magical and potentially confusing enough that I might want a message anyway. B) It may not be possible (no writable .idlerc). I am not sure what happens if not. C) If we write with the same name, 'IDLE Dark', then two things have the same name but may or may not be identical. (Users could modify the custom version of IDLE Dark.) This is why the interface demands a new name. Your patch assumes that they stay identical. D) If we write as a different name, users need to be notified anyway. Might as well let them choose, so they remember. E) A custom version is not needed for students in a class with only one IDLE on the machine. F) We would have to look at possible interactions with other parts of the dialog's behavior.

    "See below", time pressure: I though 3.4.4c1 was due out today. It has not appeared, and there have been no pydev messages for 20 hours, since I posted an enquiry out the release. If the release is a week away, we can potentially change the patch, though I would want to test very carefully.

    "See below", date-specific user config: because all releases after today will have IDLE Dark, the needed marker is date rather than version. Right now, selecting 'IDLE Dark' as a built-in theme results in user config-main.cfg having this section.

    [Theme]
    default = True
    name = IDLE Dark

    Suppose instead the result were

    [Theme]
    default = True
    name2 = IDLE Dark
    #or
    [Theme]
    default = True
    name = IDLE New
    name2 = IDLE Dark

    I tested with 2.7.10 and it starts fine with either of the above, and just as important, writes the above when Classic or New are selected if the name2 line is there. I expect the both should be true of all versions that recognize user config files, especially the startup part.

    The challenge would be to have new IDLEs add the name2 line when IDLE Dark is selected and have it replace name with name2 when fetching the name. The message could then be something to the effect of "IDLE Dark only comes with IDLE releases in Oct, 2015 and later. To make it available to older releases, save it as a custom theme (with a new name)."

    This is a bit clumsy, and would have to be generalized for further theme (but YAGNI for now), but would eliminate errors and no color with old versions. What do you think?

    @roseman
    Copy link
    Mannequin Author

    roseman mannequin commented Oct 5, 2015

    I see the 3.4.4 is not an immediate concern, so that's good.

    FYI, I get the (multiple) error messages on console consistently on Mac, but it probably depends how it was launched. Agree the code for future versions should do a better job to detect the theme isn't present and switch back to a default theme rather than just puking out error messages as it tries to get each element of the non-existent theme. :-)

    I still think the approach I took may be viable. It doesn't require changes to old versions, and relies on the fact that a user theme with the same name as a default theme is effectively hidden by the default theme.

    If a 'new' theme is selected, it's always written out to config-highlight, so it will be present for older versions (not errors and the internal default colors). Given that, I need help understanding what the 'name' vs. 'name2' etc. solution gives us?

    (Not being able to write .idlerc wouldn't be an issue, because then you wouldn't be able to write out the change to say that is the theme to use anyway).

    The only drawback I see is that a user running a previous version can customize IDLE Dark and those changes would not appear when using a newer versions of IDLE that had IDLE Dark as a default.

    This would negatively affect only those users who change to IDLE Dark, actually modify the theme, and switch back and forth between versions of IDLE. The consequences of this occurring are only that the changes don't show up. In other words, I feel this affects a very tiny number of people, in a relatively minor way.

    The existing warning on choosing themes will be encountered by anyone who might switch themes in the new version, and particularly for the non-hardcore, would be at the least surprising if not confusing or alarming. I think it's safe to assume that far more people try existing themes vs. create custom themes and use multiple versions of IDLE.

    Given the very minor consequences and how few people it would affect, vs. how many people would encounter that warning and the impression it would leave, this seems a case of I'd rather be unobtrusive and 98+% correct then obtrusive and 100% correct.

    If it were my product, there's no question I'd drop the warning under those circumstances, and if it were a choice between including the warning or not including a new theme, I'd personally choose to not include the new theme.

    Anyway, it seems we have time to think more about this...

    @roseman
    Copy link
    Mannequin Author

    roseman mannequin commented Oct 6, 2015

    On a more direct note, have attached missingtheme.patch, which ensures that all queries for the active theme go through CurrentTheme(), which has been modified to verify that the theme exists; if not, it returns 'IDLE Classic'

    @terryjreedy
    Copy link
    Member

    Schedule update: 3.4.4 will be delayed untill after 3.5.1, which will be much sooner than the original minimum of 4 months after 3.5.0. Larry promises at least two weeks notice before 3.5.1rc1 but will give no hint of when notice might be given. So we have at least two week to do something correct.

    I am against duplicating names because it is wrong in principle. On the other hand, I understand the objection to a popup always and forever. It is past time anyway to enable a working [Help] button, bpo-22726. The current popup help text should go there as tab-specific text. The only automatic notice on selecting a new builtin theme would be 'New theme, see Help', maybe in orange (warning color, but not emergency red ;-) added in the empty space below.

    I believe one new 'name2 = new_theme' addition to config-main would be enough for future additions. We add to configHandler 'new_themes, a set, currently {'IDLE Dark'}. If a selected builtin is in the set, the message is displayed and name2 is written. On reading, if name2 is present but not recognized, revert to IDLE Classic, which is present on all systems now.

    Intended result: all IDLEs keep working, new releases get a minimally intrusive notice pointing to a full explanation.

    @roseman
    Copy link
    Mannequin Author

    roseman mannequin commented Oct 7, 2015

    Okay, that's reasonable enough for me. :-) I'd still be for nuking the warning so that nothing gets displayed unless you go looking in online help.

    @terryjreedy
    Copy link
    Member

    Here is patch with a subdued message to see Help when IDLE Dark is selected. It goes away when something else is selected. I tried no emphasis, colored background, relief, and the dull red text in the patch. I tested, but I want to retest systematically when fresh. Test on *nix would good, though I do not think anything is system specific.

    Under some conditions, entry 'new = ' is being put in user config-main.cfg [Theme] section. It seems harmless but I hope to track it down, while retesting, before committing. It might just be something I did in my installed idlelib, but I'd like to know.

    @terryjreedy
    Copy link
    Member

    I also want to document new Current Theme in docstring and maybe comments for future readers.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 12, 2015

    New changeset b95c10cb457d by Terry Jan Reedy in branch '2.7':
    Issue bpo-25313: Change the handling of new built-in text color themes to better
    https://hg.python.org/cpython/rev/b95c10cb457d

    New changeset 6185c5603eed by Terry Jan Reedy in branch '3.4':
    Issue bpo-25313: Change the handling of new built-in text color themes to better
    https://hg.python.org/cpython/rev/6185c5603eed

    @terryjreedy
    Copy link
    Member

    The patch applied leaves 'name = IDLE New' alone so older IDLEs can use that instead of IDLE Classic as an alternative to IDLE Dark. I will open an issue to revise IDLE New slightly. I also added type='bool' when getting 'default' (the lack thereof lead to IDLE Dark being returned as a custom theme.

    After working with this, I like the idea of present users with a single list of themes to select, instead of two buttons and two drop-down lists.

    I tested fairly thoroughly, starting with no user [Theme] section. When I added a print to debug idleConf.CurrentTheme, it was called 13 times on startup, with only Shell opened.

    @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
    topic-IDLE type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant