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

wxGUI/history: Create panel displaying info about command #3365

Merged
merged 22 commits into from
Feb 18, 2024

Conversation

lindakarlovska
Copy link
Contributor

@lindakarlovska lindakarlovska commented Jan 16, 2024

This PR implements new functionality to the History Browser panel, particularly showing info about a command from history through the new Command Info dialog.

  • The history log (.wxgui_history) is newly stored as a JSON file. It is backward compatible with the plain .wxgui_history. But in the case that a user creates a new mapset, a new history file is automatically created as a JSON file.

  • This PR further implements a new option in the context menu called "Show Info." "Show Info" displays a small dialog called Command info about the respective process stored in the JSON file.

  • JSON file stores the following info for each executed command:

  1. Execution time
  2. Runtime duration
  3. Status (In process / Successfully finished/ Failed)
  4. If a 2D mask was set
  5. Executor login
  6. Region settings (separate notebook page in Command info dialog)

Screenshot from 2024-01-16 21-45-28

Example of the JSON structure:

[
  {
    "command": "v.what map=boundaries@PERMANENT coordinates=1.0658362989323844,0.7413997627520759",
    "command_info": {
      "Execution time": "Mon Jan 22 10:08:16",
      "Runtime duration": "0 sec",
      "Status code": "Successfully finished",
      "2D mask set": true,
      "Executor login": "linduska",
      "Region settings": {
        "projection": "99 (ETRS89-extended / LAEA Europe)",
        "zone": "0",
        "datum": "etrs89",
        "ellipsoid": "grs80",
        "north": "1",
        "south": "0",
        "west": "0",
        "east": "1",
        "nsres": "1",
        "ewres": "1",
        "rows": "1",
        "cols": "1",
        "cells": "1"
      }
    }
  },
  {
    "command": "v.what map=boundaries@PERMANENT coordinates=0.8190984578884934,0.6500593119810201",
    "command_info": {
      "Execution time": "Mon Jan 22 10:09:09",
      "Runtime duration": "0 sec",
      "Status code": "Successfully finished",
      "2D mask set": true,
      "Executor login": "linduska",
      "Region settings": {
        "projection": "99 (ETRS89-extended / LAEA Europe)",
        "zone": "0",
        "datum": "etrs89",
        "ellipsoid": "grs80",
        "north": "3017225",
        "south": "3000000",
        "west": "4623050",
        "east": "4657625",
        "nsres": "25",
        "ewres": "25",
        "rows": "689",
        "cols": "1383",
        "cells": "952887"
      }
    }
  }
]

@github-actions github-actions bot added GUI wxGUI related Python Related code is in Python libraries labels Jan 16, 2024
@lindakarlovska lindakarlovska marked this pull request as draft January 16, 2024 20:53
@lindakarlovska lindakarlovska added the enhancement New feature or request label Jan 16, 2024
@lindakarlovska lindakarlovska marked this pull request as ready for review January 22, 2024 13:58
@landam landam added this to the 8.4.0 milestone Jan 22, 2024
@petrasovaa
Copy link
Contributor

I would revise the JSON format, generally I would use lower case and leave formatting of time to GUI.

  • "Execution time" -> "timestamp" and use something like ISO 8601 format (right now there is not even a year there)
  • "Runtime duration" -> "runtime" maybe use integer representing number of seconds
  • "Status code" -> "status" could be one of failed/success/aborted
  • "2D mask set" -> "mask"
  • "Executor login": not sure how useful this is, get_mapset_owner would always give the same result for all commands (None on Windows), maybe use something different?
  • "Region settings" -> "region", values should be floats/integer, not strings, and I would drop the lines that refer to projection. I don't know why g.region even prints that.

Then in the GUI the labels should be translatable. So for example JSON will have status "success" and GUI will use _("Success").

It would be better to have the command itself in a parsed format.

Then regarding the dialog, wouldn't it be better to display the information within the history browser, e.g. at the bottom? What would be even nicer I think is to expand the command node and show the info below the command, but maybe we don't need to do this in this PR.

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Jan 23, 2024

I would revise the JSON format, generally I would use lower case and leave formatting of time to GUI.

* "Execution time" -> "timestamp" and use something like ISO 8601 format (right now there is not even a year there)

* "Runtime duration" -> "runtime" maybe use integer representing number of seconds

* "Status code" -> "status" could be one of failed/success/aborted

* "2D mask set" -> "mask"

* "Executor login": not sure how useful this is, get_mapset_owner would always give the same result for all commands (None on Windows), maybe use something different?

* "Region settings" -> "region", values should be floats/integer, not strings, and I would drop the lines that refer to projection. I don't know why g.region even prints that.

Then in the GUI the labels should be translatable. So for example JSON will have status "success" and GUI will use _("Success").

It would be better to have the command itself in a parsed format.

Thanks Anna for review. Ok, I will focus more on a JSON file and incorporate suggested stuff.

Then regarding the dialog, wouldn't it be better to display the information within the history browser, e.g. at the bottom? What would be even nicer I think is to expand the command node and show the info below the command, but maybe we don't need to do this in this PR.

Yes @petrasovaa , you are right, The Command Info dialog is quite hidden for users, to have the info directly accessible at the bottom of the History browser would be better. What do you think about the structure of the dialog content? Are two notebooks "General info" and "Region settings" okay, or would you devise something different?

As I was thinking about your suggestions, I also tried to focus more on the better user experience and I think it would be also nice to have other functionalities as part of this PR:

  1. Info about the command is shown as part of OnItemSelected event (no need for context menu option)
  2. When the command is launched by "Run", the new entry in the history browser is automatically selected (info is automatically displayed) - then we really do not need to have the "In process" status, since it is quite clear that if the status is not filled, the command is still running
  3. After GRASS startup, the last entry in the history browser is automatically selected (and its info is displayed)

I agree that expanding the command node is an interesting functionality for the next PR.

@petrasovaa
Copy link
Contributor

petrasovaa commented Jan 23, 2024

Then regarding the dialog, wouldn't it be better to display the information within the history browser, e.g. at the bottom? What would be even nicer I think is to expand the command node and show the info below the command, but maybe we don't need to do this in this PR.

Yes @petrasovaa , you are right, The Command Info dialog is quite hidden for users, to have the info directly accessible at the bottom of the History browser would be better. What do you think about the structure of the dialog content? Are two notebooks "General info" and "Region settings" okay, or would you devise something different?

I am not completely sure, I would perhaps merge it into one panel. Then place it at the bottom, similarly to how command console has it, and include a vertical scrollbar, so the region info would be hidden most of the time, which is fine. Eventually, it may be good to have an option (a button?) to set the region based on the command. It brings up a question when you rerun the command, whether it should rerun it with the same region or the current one (probably current one).

As I was thinking about your suggestions, I also tried to focus more on the better user experience and I think it would be also nice to have other functionalities as part of this PR:

  1. Info about the command is shown as part of OnItemSelected event (no need for context menu option)

yes

  1. When the command is launched by "Run", the new entry in the history browser is automatically selected (info is automatically displayed) - then we really do not need to have the "In process" status, since it is quite clear that if the status is not filled, the command is still running

AFAIU yes

  1. After GRASS startup, the last entry in the history browser is automatically selected (and its info is displayed)

I don't think you have to have that, the info will be empty, which is fine.

I agree that expanding the command node is an interesting functionality for the next PR.

@lindakarlovska
Copy link
Contributor Author

I would revise the JSON format, generally I would use lower case and leave formatting of time to GUI.

* "Execution time" -> "timestamp" and use something like ISO 8601 format (right now there is not even a year there)

* "Runtime duration" -> "runtime" maybe use integer representing number of seconds

* "Status code" -> "status" could be one of failed/success/aborted

* "2D mask set" -> "mask"

* "Executor login": not sure how useful this is, get_mapset_owner would always give the same result for all commands (None on Windows), maybe use something different?

* "Region settings" -> "region", values should be floats/integer, not strings, and I would drop the lines that refer to projection. I don't know why g.region even prints that.

Then in the GUI the labels should be translatable. So for example JSON will have status "success" and GUI will use _("Success").

It would be better to have the command itself in a parsed format.

Then regarding the dialog, wouldn't it be better to display the information within the history browser, e.g. at the bottom? What would be even nicer I think is to expand the command node and show the info below the command, but maybe we don't need to do this in this PR.

What about 3D mask? Make it sense to have it here as well as the attribute?

@lindakarlovska
Copy link
Contributor Author

I would revise the JSON format, generally I would use lower case and leave formatting of time to GUI.

* "Execution time" -> "timestamp" and use something like ISO 8601 format (right now there is not even a year there)

* "Runtime duration" -> "runtime" maybe use integer representing number of seconds

* "Status code" -> "status" could be one of failed/success/aborted

* "2D mask set" -> "mask"

* "Executor login": not sure how useful this is, get_mapset_owner would always give the same result for all commands (None on Windows), maybe use something different?

* "Region settings" -> "region", values should be floats/integer, not strings, and I would drop the lines that refer to projection. I don't know why g.region even prints that.

I used g.region with the -g flag instead of the previously used -p flag. There's still some info about the projection but only "projection" and "zone" and both as integers ("datum" and "ellipsoid" are missing). I can drop it but I think that it is more consistent just to preserve the output from g.region as it is when it adds only these two extra info..

The JSON structure looks as follows now (the command itself will be parsed in the next step).

[
  {
    "command": "v.what map=boundaries@PERMANENT coordinates=4632304.615724382,3013759.0437279153",
    "command_info": {
      "timestamp": "2024-01-25T15:56:19.743376",
      "mask": false,
      "region": {
        "projection": 99,
        "zone": 0,
        "n": 3017225,
        "s": 3000000,
        "w": 4623050,
        "e": 4657625,
        "nsres": 25,
        "ewres": 25,
        "rows": 689,
        "cols": 1383,
        "cells": 952887
      },
      "runtime": 0,
      "status": "success"
    }
  }
]

Then in the GUI the labels should be translatable. So for example JSON will have status "success" and GUI will use _("Success").

It would be better to have the command itself in a parsed format.

Then regarding the dialog, wouldn't it be better to display the information within the history browser, e.g. at the bottom? What would be even nicer I think is to expand the command node and show the info below the command, but maybe we don't need to do this in this PR.

@petrasovaa
Copy link
Contributor

What about 3D mask? Make it sense to have it here as well as the attribute?

Yes, although it's low priority. I wonder if the GUI should show the info about mask only when it was active to keep the command info short and relevant.

@petrasovaa
Copy link
Contributor

I used g.region with the -g flag instead of the previously used -p flag. There's still some info about the projection but only "projection" and "zone" and both as integers ("datum" and "ellipsoid" are missing). I can drop it but I think that it is more consistent just to preserve the output from g.region as it is when it adds only these two extra info..

I have no idea why this even gets printed, but that's a separate issue. Maybe keep it in the JSON file, but I wouldn't display this information in the GUI to the user.

@lindakarlovska lindakarlovska marked this pull request as draft January 29, 2024 21:00
@lindakarlovska
Copy link
Contributor Author

I am sharing the picture of the current version of GUI (implementation itself works but the code needs some cleaning and refactoring):
Screenshot from 2024-01-30 11-19-18

After clicking on the command, the info appears at the bottom part of the History pane.
What do you think about the current proposal @petrasovaa/ @tmszi ?

@lindakarlovska lindakarlovska changed the title wxGUI/history: Show history info dialog wxGUI/history: Create panel displaying info about command Jan 30, 2024
@tmszi
Copy link
Member

tmszi commented Jan 30, 2024

After clicking on the command, the info appears at the bottom part of the History pane. What do you think about the current proposal @petrasovaa/ @tmszi ?

Looks good to me.

Some info from the my quickly testing:

  1. During my testing sometimes d.vect command has General info with Timestamp info only, and sometimes has extended info as Runtime Duration and Status.
  2. When I remove the selected command item from the history tree, the info panel should also be empty
  3. Is there some command/internal method/func which provide transition from existed old simple .wxgui_history file to the new JSON format? Because with old simple history file, History panel command history tree items is not correctly rendered and it is not working as expected.

@petrasovaa
Copy link
Contributor

I didn't test, but it looks fine. I wouldn't spend too much time on the look of the GUI yet, it would be best to get this PR in and then see if we want to change the appearance. Thanks @tmszi for testing!

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Feb 2, 2024

To point 3 - I fixed it, but generally I was not much convinced about the solution I made (the new JSON file is created only when a user creates a new mapset) so yesterday I discussed it with @landam and we decided to automatically convert the plain text file to JSON file when launching a new command. This is however a bit problematic regarding the fact that we wanted to have the parsed command in a JSON file (as list - I think @petrasovaa suggested it), so whenever this conversion would happen it would require parsing all commands from old plain-text history log into lists. So, from this point of view, it would be better to preserve the logic of commands stored as strings in the JSON file similarly as in the plain text. So we will basically have two "read" methods (from JSON and from plain-text) but only one write method which will always write into JSON. I will change the current solution to this variant in a separate commit.

Sounds good to me. Unrelated, but looking at the failed Python code quality test, it would be good to setup pre-commit so you avoid this.

@lindakarlovska lindakarlovska marked this pull request as ready for review February 2, 2024 17:03
@lindakarlovska lindakarlovska self-assigned this Feb 7, 2024
python/grass/grassdb/history.py Outdated Show resolved Hide resolved
python/grass/grassdb/history.py Outdated Show resolved Hide resolved
gui/wxpython/gui_core/prompt.py Outdated Show resolved Hide resolved
gui/wxpython/history/browser.py Outdated Show resolved Hide resolved
gui/wxpython/history/browser.py Outdated Show resolved Hide resolved
petrasovaa
petrasovaa previously approved these changes Feb 14, 2024
@lindakarlovska lindakarlovska merged commit 4d2dac4 into OSGeo:main Feb 18, 2024
25 checks passed
jadenabrams100 pushed a commit to ncsu-csc472-spring2024/grass-CI-playground that referenced this pull request Feb 21, 2024
The history log .wxgui_history is newly stored as a JSON file with command and command_info keys. When a new command is launched, it is stored as a new entry to a JSON file. The old plain text is preserved but not further used (we have two read methods but only one write method).

The command info contains the following:
runtime, timestamp, status (success, failed, aborted), if 2D/3D is set, and region settings.

The History browser is newly the object of the Splitter window. At the upper part, there is a tree catalog with history commands, and at the bottom part, there is a panel displaying the command info.


---------

Co-authored-by: lindakladivova <l.kladivova@seznam.cz>
Co-authored-by: Anna Petrasova <kratochanna@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request GUI wxGUI related libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants