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

notes script #1287

Merged
merged 15 commits into from
Sep 6, 2024
Merged

notes script #1287

merged 15 commits into from
Sep 6, 2024

Conversation

wiktor-obrebski
Copy link
Contributor

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

Notes

This PR introduce notes feature that allows to add simple notes to the game map.

screen

Features:

  • notes add to add new note to the map (in place of keyboard cursor)
  • enable notes.map_notes overlay to render notes on the map as green pins
  • click on map notes to open note details and edit screen
  • click few times to toggle between notes if they are all in the same tile
  • the overlay has been written with performance in mind (and profiled)

TODO:

  • documentation
  • way to trigger note modal in ASCII mode
  • one-line-mode for text editor, for the note name
  • move ? button from the text area component to the journal itself

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.

I'm unsure whether this should be its own tool or whether it should be a tab in gui/journal. Maybe that could take some more discussion on Discord.

One thing that is missing from this interface is a way to search existing notes by name.

internal/journal/text_editor.lua Outdated Show resolved Hide resolved
notes.lua Outdated Show resolved Hide resolved
notes.lua Outdated Show resolved Hide resolved
notes.lua Outdated Show resolved Hide resolved
notes.lua Outdated

local last_note_on_pos = nil
local first_note_on_pos = nil
for _, note in pairs(notes) do
Copy link
Member

Choose a reason for hiding this comment

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

The complexity of this loop (which should be using ipairs, btw) will grow with the number of notes/squad waypoints that are defined. would it be a good idea to cache them in a nested map structure so you can retrieve a note via safe_index(self.notes_cache, pos.z, pos.y, pos.x)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I optimized it now in different way - I scan only notes that are visible (and cached before). What do you think?

notes.lua Outdated Show resolved Hide resolved
notes.lua Outdated
function NoteManager:createNote()
local x, y, z = pos2xyz(df.global.cursor)
if x == nil then
print('Enable keyboard cursor to add a note.')
Copy link
Member

Choose a reason for hiding this comment

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

A mouse-capable way of adding a note is generally expected of current tools. See gui/blueprint for a possible example. You click on a button and then click on the map. while the location is being selected, a marker is rendered under the mouse cursor: https://github.com/DFHack/scripts/blob/master/gui/blueprint.lua#L427

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems very good, but for now we do not have any notes interface where such button can be placed.
Do you think I should replace notes add command by mouse cursor selection?
Or this should stay as it is and the new way should appear after there will be a gui/notes for managing notes?

Copy link
Member

Choose a reason for hiding this comment

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

I think there should be a gui/notes entrypoint, but I also think that this dialog should have a way to create (and switch to editing) an additional note

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be confusing.

  1. You clicked on a note to edit it and in a dialog you have option "New note"? Did you current changes to note saved or not? Do it switch to new note overlay mouse hover mode? etc.

  2. You creating a new note, and got to moment when you see this dialog. Then you switch to editing. What this really means? You are forced to select a note on the screen now? Its sounds very confusing.
    If you want to switch to editing - just select a note on the map. You can do this right now, do not need any additional wat to "switch" to it.
    --
    I never met such features in my web/desktop experience. Edit dialog is for editing, new dialog is for creating.

I believe the needs you speak about will be fully solved by gui/notes tool.
I though I can create it later, but maybe it should be done from the begining to avoid the confusion.

notes.lua Outdated Show resolved Hide resolved
notes.lua Outdated Show resolved Hide resolved
notes.lua Outdated Show resolved Hide resolved
notes.lua Show resolved Hide resolved
notes.lua Show resolved Hide resolved
notes.lua Outdated Show resolved Hide resolved
notes.lua Outdated Show resolved Hide resolved
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.

needs docs/notes.rst and New tools line in changelog.txt

@myk002
Copy link
Member

myk002 commented Sep 4, 2024

regarding this PR title: this is a script, not a plugin. DFHack "plugins" are C++

@wiktor-obrebski wiktor-obrebski changed the title notes plugin notes script Sep 4, 2024
@wiktor-obrebski wiktor-obrebski marked this pull request as ready for review September 4, 2024 17:23
docs/notes.rst Outdated Show resolved Hide resolved
docs/notes.rst Outdated Show resolved Hide resolved
docs/notes.rst Outdated Show resolved Hide resolved
@myk002 myk002 merged commit 22675fc into DFHack:master Sep 6, 2024
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
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants