Skip to content

Fix width argument, set overall width#825

Merged
dbarnett merged 2 commits intoinsanum:mainfrom
msalle:fix/width_agenda_upstream
Jan 28, 2025
Merged

Fix width argument, set overall width#825
dbarnett merged 2 commits intoinsanum:mainfrom
msalle:fix/width_agenda_upstream

Conversation

@msalle
Copy link
Copy Markdown
Contributor

@msalle msalle commented Jan 21, 2025

Fix the width for the description details, both default and as cmdline option, and make sure we don't break the width of the calw and calm output.
The (pretty much undocumented) option --width/-w option was used for setting the width of a day table cell in calw and calm but seemed to also be intended for setting the overall width of the description in agenda. The latter part was broken and the details description width would always default to 80.
We change the behaviour to set the overall width of the table, defaulting to the terminal width, both for calw and calm and for description details. The width of the day cell is calculated from the overall width.

Fix the width for the description details, both default and as cmdline option,
and make sure we don't break the width of the calw and calm output.
The (pretty much undocumented) option --width/-w option was used for setting the
width of a day table cell in calw and calm but seemed to also be intended for
setting the overall width of the description in agenda. The latter part was
broken and the details description width would always default to 80.
We change the behaviour to set the overall width of the table, defaulting to the
terminal width, both for calw and calm and for description details. The width of
the day cell is calculated from the overall width.


def validwidth(value):
minwidth=30
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This change from 10 to 30 is because 10 was just intended as the min size for one day cell vs whole output? Is there a specific reason for picking 30?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's kind of the minimal width that still makes any sense for gcalcli agenda --details description. For 30 it comes out to about a width of 5 chars for the description. But I'm happy to change it to anything else obviously.

Mon Jan 27  8:30           Strategy meeting
                     Description:
                     ┌───────┐
                     │ aaaaa │
                     │ aaaaa │
                     │ aaaaa │
                     │ aaaaa │

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nah I doubt it'll bother anyone, just wanted to understand if it's an intentional behavior change.

@dbarnett
Copy link
Copy Markdown
Collaborator

Would you mind giving an example of output before/after for reference?

And I see some failed lint checks for trivial spelling/formatting if you could fix those up.

@msalle
Copy link
Copy Markdown
Contributor Author

msalle commented Jan 27, 2025

Would you mind giving an example of output before/after for reference?

The old behaviour would be something like

Mon Jan 27  8:30           Strategy meeting
                     Description:
                     ┌─────────────────────────────────────────────────────────┐
                     │ abcdefghijklmnopqstuvwxyz.............................. │
                     │ ................................                        │
                     └─────────────────────────────────────────────────────────┘

irrespective of terminal size or given --width ... argument (i.e. always 80chars terminal basically), where the new behaviour for the same event becomes

Mon Jan 27  8:30           Strategy meeting
                     Description:
                     ┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
                     │ abcdefghijklmnopqstuvwxyz..............................................................                                                                                                                             │
                     └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

depending on the terminal size when no --width argument is given. That behaviour is the same as --width $COLUMNS (which is useful when the output is not a terminal but e.g. a pipe.

The output for calw and calm isn't changed except that the --width argument now sets the overall width as it does for agenda. So

gcalcli calw --width $COLUMNS

would be a logical thing to write.

Let me know if you like more detailed examples.

And I see some failed lint checks for trivial spelling/formatting if you could fix those up.

Those should be fixed. I've changed the long line also to match the formatting and use of %d and % as in validreminder


# Store overall calendar width and width for day table cells
self.width['cal'] = int(options.get('width', 80))
day_width = int(( self.width['cal'] - 8) / 7)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FYI dividing by 7 probably doesn't handle --noweekend exactly right, but looks like that's a preexisting issue and minor.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you're right with both: it's indeed not handled correctly and is a preexisting issue, but it got moved in this PR.
I've just opened a new PR #826 to fix it.

@dbarnett dbarnett merged commit e0e126e into insanum:main Jan 28, 2025
10 checks passed
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