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

CSSTUDIO-2572: Add a "Jump to Log Entry" TextField to the Logbook application #3195

Merged
merged 12 commits into from
Nov 20, 2024

Conversation

abrahamwolk
Copy link
Collaborator

This pull request adds a "Jump to Log Entry" textfield to the Logbook application:
screenshot

By writing a Log Entry ID into this textfield, one can jump to the corresponding log entry.

@kasemir
Copy link
Collaborator

kasemir commented Nov 19, 2024

How likely it is for an end user to know the ID of a log entry? A plain text search might be more relevant, where entries with that text in the author, title, body will be highlighted, with next/previous buttons to jump between log entries that contain the word.

Otherwise fine.

@abrahamwolk
Copy link
Collaborator Author

abrahamwolk commented Nov 19, 2024

@kasemir wrote:

How likely it is for an end user to know the ID of a log entry? A plain text search might be more relevant, where entries with that text in the author, title, body will be highlighted, with next/previous buttons to jump between log entries that contain the word.

The ID of a log entry is displayed when displaying the log entry:
screenshot

The use case is that a user wants to share a link (or equivalent) to a log entry when using the Phoebus Logbook client. It is not practical to ask another user to search for the log entry in question.

@georgweiss
Copy link
Collaborator

It is already possible to share links: see Copy URL button, which is also available in the web client.

@abrahamwolk
Copy link
Collaborator Author

It is already possible to share links: see Copy URL button, which is also available in the web client.

Sharing is currently possible, but opening a link in the Logbook application is not possible. This pull request is intended to allow a user to open a specified log entry in the Logbook application in Phoebus.

Copy link
Collaborator

@georgweiss georgweiss left a comment

Choose a reason for hiding this comment

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

Log entry ids are always numerical, so the input field should accept only digits.
Also, the cursor is aligned in the middle and after uses presses it jumps back to the start of the entered value.

@abrahamwolk
Copy link
Collaborator Author

Log entry ids are always numerical, so the input field should accept only digits.

If the parsing of the log entry ID fails (e.g., the input contains non-numeric characters), the input is discarded:
https://github.com/ControlSystemStudio/phoebus/pull/3195/files#diff-dadeacf8698c9fa7ab9b014b3b71d535270075a70af2c586c08638f06665e59dR156-R161

Also, the cursor is aligned in the middle and after uses presses it jumps back to the start of the entered value.

Do I understand you correctly: after entering a value, you would prefer the the cursor to be placed to the right of the entered value? If so, I agree with that and I think it would be preferable.

@georgweiss
Copy link
Collaborator

Using this new feature and then Back button. Eventually the history is empty, which is indicated by a disabled Back button. At this point I think the input field should be cleared too.
Screenshot 2024-11-19 at 14 19 27

@georgweiss
Copy link
Collaborator

If the parsing of the log entry ID fails (e.g., the input contains non-numeric characters), the input is discarded

In my view disallowing invalid input is nicer. That would also eliminate the need to handle the invalid input.

And yes, I think right-aligning cursor is more intuitive. And the entered chars should be left-aligned, in my opinion.

@abrahamwolk
Copy link
Collaborator Author

Using this new feature and then Back button. Eventually the history is empty, which is indicated by a disabled Back button. At this point I think the input field should be cleared too. Screenshot 2024-11-19 at 14 19 27

I don't think so: isn't 451 the ID of the log entry that is currently being displayed? The "Back" button is disabled since there is no previous log entry to go back to.

@abrahamwolk
Copy link
Collaborator Author

In my view disallowing invalid input is nicer. That would also eliminate the need to handle the invalid input.

How would you realize that? One possibility seems to be to use a TextFormatter. However, I think that is more complicated than the current solution, and the current solution works well, I think.

@georgweiss
Copy link
Collaborator

When reaching (in my example) the "last" id 451, the Forward button will take user to 451.

@abrahamwolk
Copy link
Collaborator Author

I have now updated the pull request with the following changes:

  1. The text in the "Jump to Log Entry" TextField is now aligned to the left.
  2. A condition that the entered value for the log entry ID is greater than 0 is checked, and the error handling when a log entry doesn't load successfully is improved.
  3. The caret is moved to the end of the TextField after a value has been entered.

@abrahamwolk
Copy link
Collaborator Author

abrahamwolk commented Nov 19, 2024

When reaching (in my example) the "last" id 451, the Forward button will take user to 451.

The textfield should always display the ID of the current log entry. If it doesn't, that is a bug. Is that the case?

…k action when a log entry was successfully retrieved.
@abrahamwolk
Copy link
Collaborator Author

The textfield should always display the ID of the current log entry. If it doesn't, that is a bug. Is that the case?

As you demonstrated to me yesterday, there was an underlying bug: when trying to go to a log entry that doesn't exist, a "go back" action was added to the log entry that was already being displayed. The result is that the current log entry had two entries in the history.

I have now fixed this by only clearing the forward history and adding "go back" actions to the history when the retrieval of a log entry was successful.

@abrahamwolk abrahamwolk merged commit 8559774 into master Nov 20, 2024
2 checks passed
@abrahamwolk abrahamwolk deleted the CSSTUDIO-2572 branch November 20, 2024 08:21
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.

3 participants