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

Highlight alignment column #525

Merged
merged 1 commit into from
Nov 17, 2024

Conversation

tangledhelix
Copy link
Collaborator

Adds "Highlight Alignment Column" to the Search menu. This should work the same way the GG1 feature did: when activated, whatever column the cursor is on will be set as the alignment column and highlighted with a color, until turned off.

I re-used the same color as GG1 in light mode, and chose a more muted shade for the dark theme.

(Marking this as a draft for now; until #519 is merged, this PR will have all the diffs from that PR, because this branch is based on that other branch.)

Fixes #43

@tangledhelix tangledhelix added core feature Required for basic PPing future feature New feature or request, but not core and removed core feature Required for basic PPing future feature New feature or request, but not core labels Nov 14, 2024
@tangledhelix
Copy link
Collaborator Author

Removing draft now that #519 is merged.

@tangledhelix tangledhelix marked this pull request as ready for review November 16, 2024 00:44
Copy link
Member

@srjfoo srjfoo left a comment

Choose a reason for hiding this comment

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

A couple of comments.

It appears that just moving the cursor does not move the alignment column, which, when I think about it, makes sense. However, as it is right now, if you want to move the highlighted column, you need to toggle it off and back on again. I'm wondering if there's a better way, or if we just need to live with it?

And a question for both @tangledhelix and @windymilla -- "Remove Highlights" is still the last Highlight-related menu item. It applies only to the first two Highlight-related menu items. It seems rather confusing to me to have it below two menu items that it doesn't affect at all. (I know it doesn't relate directly to this PR, but I had already noticed it, and now there's yet another menu item separating it from those items it actually affects.

None of my comments are things that need to be solved right now.

src/guiguts/highlight.py Show resolved Hide resolved
src/guiguts/highlight.py Outdated Show resolved Hide resolved
@tangledhelix
Copy link
Collaborator Author

It appears that just moving the cursor does not move the alignment column, which, when I think about it, makes sense. However, as it is right now, if you want to move the highlighted column, you need to toggle it off and back on again. I'm wondering if there's a better way, or if we just need to live with it?

I'm not sure of a more direct way to move the column unless we broke this into 2 menu items - "set column" and "disable column". Then you could set the column and it would move it even if it were already on, and to turn off entirely you'd disable. I don't use this feature in GG1 myself so I have no remark as to how PPers use it in real work and whether moving it is a common operation.

And a question for both @tangledhelix and @windymilla -- "Remove Highlights" is still the last Highlight-related menu item. It applies only to the first two Highlight-related menu items.

Currently, yes. When #41 is implemented it will also relate to that.

It seems rather confusing to me to have it below two menu items that it doesn't affect at all. (I know it doesn't relate directly to this PR, but I had already noticed it, and now there's yet another menu item separating it from those items it actually affects.

I'm happy to rearrange - so far I've just copied the GG1 menu positions. Perhaps these should be in a menu grouping (the 3 highlights + remove highlights, then the 2 checkbox highlight modes in a group together either before or after those?

@srjfoo
Copy link
Member

srjfoo commented Nov 16, 2024

It seems rather confusing to me to have it below two menu items that it doesn't affect at all. (I know it doesn't relate directly to this PR, but I had already noticed it, and now there's yet another menu item separating it from those items it actually affects.

I'm happy to rearrange - so far I've just copied the GG1 menu positions. Perhaps these should be in a menu grouping (the 3 highlights + remove highlights, then the 2 checkbox highlight modes in a group together either before or after those?

I like the idea of grouping them.

And a question for both @tangledhelix and @windymilla -- "Remove Highlights" is still the last Highlight-related menu item. It applies only to the first two Highlight-related menu items.

Currently, yes. When #41 is implemented it will also relate to that.

Sounds right -- and it could go with the group it relates to.

@tangledhelix
Copy link
Collaborator Author

Merge after #529 due to rebasing...

@tangledhelix tangledhelix marked this pull request as draft November 16, 2024 11:28
Copy link
Collaborator

@windymilla windymilla left a comment

Choose a reason for hiding this comment

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

I think it would be preferable if the column highlight appeared in both windows when the text window is split, for example when the user has split the screen to help align things in two separate parts of the file. Apart from minor code complication, I can't see any downsides.

@windymilla
Copy link
Collaborator

It appears that just moving the cursor does not move the alignment column, which, when I think about it, makes sense. However, as it is right now, if you want to move the highlighted column, you need to toggle it off and back on again. I'm wondering if there's a better way, or if we just need to live with it?

I'm not sure of a more direct way to move the column unless we broke this into 2 menu items - "set column" and "disable column". Then you could set the column and it would move it even if it were already on, and to turn off entirely you'd disable. I don't use this feature in GG1 myself so I have no remark as to how PPers use it in real work and whether moving it is a common operation.

In GG1, there is a keyboard shortcut (which we're generally avoiding in GG2 for the moment) which means it's an easy "double key hit" to "turn off and turn back on the new position". If you have tear-offs, it's also not too much of a nuisance. However, if you haven't torn off the menu, or your system doesn't support tear-offs, then I agree it's a bit of a pain to have to navigate twice into the menu in order to adjust the column. I imagine moving it is something that would be required quite often: if you are doing some alignment related to a table or box or ascii art, it's likely you will want to do some aligning to one column, then some to another, etc. I therefore quite like your idea of 2 menu items @tangledhelix

@tangledhelix
Copy link
Collaborator Author

In GG1, there is a keyboard shortcut (which we're generally avoiding in GG2 for the moment) which means it's an easy "double key hit" to "turn off and turn back on the new position". If you have tear-offs, it's also not too much of a nuisance. However, if you haven't torn off the menu, or your system doesn't support tear-offs, then I agree it's a bit of a pain to have to navigate twice into the menu in order to adjust the column. I imagine moving it is something that would be required quite often: if you are doing some alignment related to a table or box or ascii art, it's likely you will want to do some aligning to one column, then some to another, etc. I therefore quite like your idea of 2 menu items @tangledhelix

I'm not sure if I agree - if this is commonly used by a PPer, it seems like remembering one key that can:

  • activate column
  • deactivate column
  • move column (double-tap)

would be better for them than either two hotkeys, or one hotkey plus being forced to use the menu to deactivate. I think as a PPer I'd prefer one menu item, but with a hotkey attached for rapid change of state without menus.

@windymilla
Copy link
Collaborator

I'd prefer one menu item, but with a hotkey attached for rapid change of state without menus.

In that case, let's stick with the current situation, i.e. like GG1 apart from the lack of keyboard shortcut which we'll address later.

@srjfoo
Copy link
Member

srjfoo commented Nov 16, 2024

I'd prefer one menu item, but with a hotkey attached for rapid change of state without menus.

In that case, let's stick with the current situation, i.e. like GG1 apart from the lack of keyboard shortcut which we'll address later.

At the risk of a shout of horror ... we already allow line numbers to be turned on and off via the toolbar. Could we do the same for turning column highlighting on and off? The menu item would be there, but it would be quicker via the toolbar.

@tangledhelix
Copy link
Collaborator Author

At the risk of a shout of horror ... we already allow line numbers to be turned on and off via the toolbar. Could we do the same for turning column highlighting on and off? The menu item would be there, but it would be quicker via the toolbar.

Mainly the question is only: is there room for it? I'll defer to Nigel's opinion, I can go either way. It would be more convenient than a menu item, probably.

@windymilla
Copy link
Collaborator

At the risk of a shout of horror ... we already allow line numbers to be turned on and off via the toolbar. Could we do the same for turning column highlighting on and off? The menu item would be there, but it would be quicker via the toolbar.

Mainly the question is only: is there room for it? I'll defer to Nigel's opinion, I can go either way. It would be more convenient than a menu item, probably.

Another question when considering using the status bar is whether the feature is used often enough to justify it. In this case, I think the answer is a definite "no". The line numbers on/off is also a rarely needed feature, but has the advantage that there is an obvious (line & column) button to bind it to, so it doesn't "take up any space". I would be perfectly happy for that binding to be removed and the only way to turn line numbers on/off would be the Prefs dialog.

@windymilla
Copy link
Collaborator

The only further change I'd like is for it to work split-screen. I'm happy to defer keyboard shortcuts for now.

Copy link
Collaborator

@windymilla windymilla left a comment

Choose a reason for hiding this comment

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

Works in both windows, with cursor in either window - thanks

@windymilla
Copy link
Collaborator

Are you happy for this to be un-"draft"-ed and merged now @tangledhelix ?

@tangledhelix
Copy link
Collaborator Author

The only further change I'd like is for it to work split-screen. I'm happy to defer keyboard shortcuts for now.

I had to refactor some of the infrastructure for the quotes/brackets to make it work, but the align column should work in split mode now. As a nice side effect, the viewport coordinates calculator can now be told how many offscreen lines to consider. I defaulted it to 5, hoping that would make the align column still "smooth" while scrolling, but allowed a different value to be passed in (so quotes/brackets is still looking at 80 offscreen rows in each direction).

I might suggest re-testing quotes & brackets when looking at this. They seem to work in my testing.

Testing notes:

  • Quotes & brackets will move with the focus, in split view. That's intentional (when you move panes, the cursor focus moves with you).
  • Highlight for alignment column should be in both text panes and should match.

For now, as discussed earlier in this PR, I left a single menu item, with no hotkey (we could add one later). I also didn't mess with colors in dark mode yet, we'll address that separately as part of #532

@tangledhelix
Copy link
Collaborator Author

@windymilla Yes I am, with the one caveat that I wouldn't mind if quotes & brackets are quickly re-tested JIC. For me they are still working but no reason not to double-check it.

@tangledhelix tangledhelix marked this pull request as ready for review November 17, 2024 16:00
@windymilla
Copy link
Collaborator

@windymilla Yes I am, with the one caveat that I wouldn't mind if quotes & brackets are quickly re-tested JIC. For me they are still working but no reason not to double-check it.

Checked - both alignment column and quotes & brackets are working as expected.

@windymilla windymilla merged commit dc91312 into DistributedProofreaders:master Nov 17, 2024
1 check passed
@tangledhelix tangledhelix deleted the hl_aligncol branch November 17, 2024 16:11
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.

Highlight Alignment Column
3 participants