Skip to content

gitk: Preserve window dimensions on exit when not using ttk themes #389

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

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

Conversation

echuber2
Copy link

@echuber2 echuber2 commented Oct 10, 2019

This fix is intended to let gitk preserve the window pane dimensions regardless of whether ttk is enabled or not. I'm not an expert on Tcl/Tk but as far as I can tell, this edit works and doesn't cause problems. Please double-check what I did.

Cc: Paul Mackerras [email protected]

Bug was: gitk would overwrite the botwidth setting in .gitk with
a nonsense value when not using tk themes. I'm not sure if this
is the right fix or not but it seems to work. Moving the affected
line within the conditional results in the expected behavior.

Signed-off-by: Eric Huber <[email protected]>
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 10, 2019

Welcome to GitGitGadget

Hi @echuber2, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that this Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form /allow <username>.

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions.

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via:

curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

@echuber2
Copy link
Author

I'm not sure why these CI checks are failing (currently).

@dscho
Copy link
Member

dscho commented Oct 11, 2019

I'm not sure why these CI checks are failing (currently).

Some of them are flaky. I restarted the CI, and now all is green.

Please note that you will want to Cc: the gitk maintainer, Paul Mackerras ([email protected]), explicitly. You can do so by adding a footer to your PR description: Cc: Paul Mackerras <[email protected]>.

@dscho
Copy link
Member

dscho commented Oct 11, 2019

/allow

@dscho
Copy link
Member

dscho commented Oct 11, 2019

(Sorry for not noticing earlier that you need to get into that list of permitted GitGitGadget users...)

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 11, 2019

User echuber2 is now allowed to use GitGitGadget.

WARNING: echuber2 has no public email address set on GitHub

@echuber2
Copy link
Author

Please note that you will want to Cc: the gitk maintainer, Paul Mackerras ([email protected]), explicitly. You can do so by adding a footer to your PR description: Cc: Paul Mackerras <[email protected]>.

Thanks. Have I done so properly now?

@dscho
Copy link
Member

dscho commented Oct 12, 2019

Have I done so properly now?

I would remove the Signed-off-by: line (it really is only needed in commit messages, not in the cover letter), and I would try not to repeat in the cover letter what the patch text is already saying...

(Background: there has been talk on the Git mailing list that some people prefer 1-patch "patch series" not to have a cover letter at all, but GitGitGadget does not support that yet.)

@dscho
Copy link
Member

dscho commented Oct 12, 2019

Have I done so properly now?

I would remove the Signed-off-by: line (it really is only needed in commit messages, not in the cover letter), and I would try not to repeat in the cover letter what the patch text is already saying...

I should have clarified: yes, the addition of the Cc: line should work.

@echuber2
Copy link
Author

I've edited my top comment in this thread, assuming that's what becomes the "cover letter". Please let me know if there's anything else I need to do.

@dscho
Copy link
Member

dscho commented Oct 13, 2019

I've edited my top comment in this thread

Looks good to me!

@echuber2
Copy link
Author

All right, thanks. I'll try sending this then.

@echuber2
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 15, 2019

Submitted as [email protected]

WARNING: echuber2 has no public email address set on GitHub

@@ -2526,9 +2526,9 @@ proc makewindow {} {
bind %W <Map> {}
Copy link

Choose a reason for hiding this comment

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

On the Git mailing list, Paul Mackerras wrote (reply to this):

On Tue, Oct 15, 2019 at 12:13:16AM +0000, Eric Huber via GitGitGadget wrote:
> From: Eric Huber <[email protected]>
> 
> Bug was: gitk would overwrite the botwidth setting in .gitk with
> a nonsense value when not using tk themes. I'm not sure if this
> is the right fix or not but it seems to work. Moving the affected
> line within the conditional results in the expected behavior.
> 
> Signed-off-by: Eric Huber <[email protected]>

Thanks, patch applied.

Paul.

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