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

Feat gui/notes #1297

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

wiktor-obrebski
Copy link
Contributor

@wiktor-obrebski wiktor-obrebski commented Sep 8, 2024

screen

Changes:

  • introduce gui/notes tool that allow to search, add, update and delete notes
  • introduce Home / End shortcut to control line cursor to TextEditor
  • introduce Delete shortuct to control line cursor to TextEditor

@myk002
Copy link
Member

myk002 commented Sep 8, 2024

The multiple peer elements that can be focused makes this UI trickier than most. Here are my thoughts:

The Search box makes sense to me -- hit the hotkey, focus the field -- but none of the other fields are presented that way. I think they should be. Name and comments could be styled similarly to the Search field, with their own hotkeys. Presumably this will obviate the need for the "Edit" hotkey at the bottom -- I didn't look at the code for that, but from the UI I'm not actually sure what that does.

I am concerned about TextEditor's intrusion into EditField's use case. TextEditor is the more featureful widget, but I'd rather see EditField turned into a subclass of TextEditor than TextEditor in "single line" mode just being offered as an alternative to EditField with slightly different semantics and behavior.

I can understand why you like the style presentation you have, with the edit areas surrounded by a frame, and I'm not opposed to it. However, I don't like how this is bifurcating the single line editor UX across DFHack tools.

The Notes list is an additional wrinkle. A hotkey to focus the list can work, or you can assign hotkeys for navigating the list regardless of what other field has the focus. This is what gui/launcher does, and I think it would make this UI simpler if you follow its example. The function of the current hotkey labeled "Notes" was not clear at all to me from looking at the UI. I had to read the code to divine its purpose (focusing the notes list for keyboard input).

@wiktor-obrebski
Copy link
Contributor Author

I am concerned about TextEditor's intrusion into EditField's use case. TextEditor is the more featureful widget, but I'd rather see EditField turned into a subclass of TextEditor than TextEditor in "single line" mode just being offered as an alternative to EditField with slightly different semantics and behavior.

I agree and I am happy to follow this way if you decide we can do it. The EditField current functionality is very limited and it would be much more useful if could support most of the TextEditor features.

@wiktor-obrebski
Copy link
Contributor Author

The Notes list is an additional wrinkle. A hotkey to focus the list can work, or you can assign hotkeys for navigating the list regardless of what other field has the focus. This is what gui/launcher does, and I think it would make this UI simpler if you follow its example. The function of the current hotkey labeled "Notes" was not clear at all to me from looking at the UI. I had to read the code to divine its purpose (focusing the notes list for keyboard input).

I agree, going to change it.

@wiktor-obrebski
Copy link
Contributor Author

The Search box makes sense to me -- hit the hotkey, focus the field -- but none of the other fields are presented that way. I think they should be. Name and comments could be styled similarly to the Search field, with their own hotkeys. Presumably this will obviate the need for the "Edit" hotkey at the bottom -- I didn't look at the code for that, but from the UI I'm not actually sure what that does.
This not working that way.
The gui/notes is tool to navigate the notes, not edit/add them. Will already have a modal to edit/add notes (created in last PR).
The Edit option hide gui/notes and open note edit modal.

So, the name/comment do not need focus control because they are static labels.

Why like this?
Because there should not be more that single way to do the same thing.
So, if I find a note on the map and click it, I see the edit note modal.
The same modal should be seen here if user want to edit note.

I think its not a problem - IMO typically user will edit notes by clicking them on map.
This tool will be used to quicly find a note when you know how its called, or add new note (which I consider also adding directly as notes add command, so it can be added on a keyboard shortcut).

@myk002
Copy link
Member

myk002 commented Sep 14, 2024

I am concerned about TextEditor's intrusion into EditField's use case. TextEditor is the more featureful widget, but I'd rather see EditField turned into a subclass of TextEditor than TextEditor in "single line" mode just being offered as an alternative to EditField with slightly different semantics and behavior.

I agree and I am happy to follow this way if you decide we can do it. The EditField current functionality is very limited and it would be much more useful if could support most of the TextEditor features.

Yeah, I'm definitely not suggesting any changes about that for this PR. Once TextEditor is stable enough to move to the widgets module, we can deprecate EditField and/or replace its functionality with TextEditor

Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

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

Notes from play testing:

  • the first two characters of a search don't have an effect. You can search for ZZ and it will match all entries, regardless of whether they have any zs in them
  • Alt-s for search has precedent, but "action" hotkeys should be Ctrl hotkeys
  • I think the player should be returned to gui/notes after placing a new note, similar to the flow for editing an existing note
  • I think the displayed note name/comment text should follow the list highlight -- that it, it should be updated on on_select, not on_submit. on_submit can still control the zoom
  • I keep expecting the text on the right to be editable in-situ and not require a new window to come up, but I realize having so many focusable elements on one page might be impractical. @Ozzatron what do you think here?

gui/notes.lua Outdated
Comment on lines 30 to 35
-- allow cursor up/down to be used to navigate the notes list
if keys.KEYBOARD_CURSOR_UP or keys.KEYBOARD_CURSOR_DOWN then
return false
end

return NotesSearchField.super.onInput(self, keys)
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense for TextEditor to not handle KEYBOARD_CURSOR_UP and KEYBOARD_CURSOR_DOWN when one_line_mode=true? Then this function would not be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

row_height=1,
on_submit=function (ind, note)
self:loadNote(note)
dfhack.gui.pauseRecenter(note.point.pos)
Copy link
Member

Choose a reason for hiding this comment

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

it might be better to use dfhack.gui.revealInDwarfmodeMap(note.point.pos, true, true) (or (note.point.pos, false, true)) so the target square gets a flashing highlight. This would help identify the specific map marker if there are many in the area.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the flashlight do not work if we render something custom on the map - and we do, the note marker.
any idea how it can be fixed (if it should be fixed)?

@wiktor-obrebski
Copy link
Contributor Author

I keep expecting the text on the right to be editable in-situ and not require a new window to come up, but I realize having so many focusable elements on one page might be impractical. @Ozzatron what do you think here?

Please consider already have a need for the dedicated modal on edit to react on user clicks on the map notes.
It would be consider to have two different places to edit a note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review In Progress
Development

Successfully merging this pull request may close these issues.

2 participants