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

Delete and add logic divided, RemoveNode fixed, cmd buffer is newly updated #2

Conversation

lindakarlovska
Copy link

Update to OSGeo#3310

@tmszi tmszi self-requested a review January 4, 2024 13:17
@tmszi tmszi self-requested a review January 4, 2024 13:52
@lindakarlovska
Copy link
Author

lindakarlovska commented Jan 4, 2024

I removed the tree.py since it seems to me that it would only make sense to have the methods GetModel and FillModel here. Something like this:

class HistoryBrowserTree:
    """Data class for the history browser tree of executed commands."""

    def __init__(self):
        self._history_path = None
        self.model = TreeModel(ModuleNode)
        self.FillModel()

    def GetModel(self):
        """Returns a deep copy of the model."""
        return copy.deepcopy(self.model)
    
    def FillModel(self):
        """Fill tree history model based on the current history log from scratch."""
        self.model.RemoveNode(self.model.root)
        self._history_path = get_current_mapset_gui_history_path()
        if self._history_path:
            cmd_list = read_history(self._history_path)
            for label in cmd_list:
                data = {"command": label.strip()}
                self.model.AppendNode(
                    parent=self.model.root,
                    label=data["command"],
                    data=data,
                )

But please, feel free to set up this file again if you think it could be useful. Maybe in future it would be nice to have separate logic.. I do not have a strong opinion.

Copy link

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

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

I am not sure the signals as they are now is the best option. E.g. why the actual removing/adding to history file is done in gui_core/goutput.py? I am thinking the actual adding/removing should be done wherever it is requested, so adding in core/gconsole.py and deleting in browser.py. The signals then should be informing the action has already happened, e.g. cmdPrompt needs to updated, tree needs to be updated. Right now the signals are both requesting change in history file and informing the change is happening. I believe this would clarify it, but maybe I missed something.

@petrasovaa
Copy link

But please, feel free to set up this file again if you think it could be useful. Maybe in future it would be nice to have separate logic.. I do not have a strong opinion.

Not sure either, it is somewhat confusing that the class is named ...Tree, but in fact is more of a manager of the TreeModel. So maybe in the future the class could be actually inheriting from the TreeModel. In datacatalog the class DataCatalogTree inherits from TreeView, so we are overall inconsistent, so perhaps we should later restructure this in a similar way, at least for consistency.

@lindakarlovska
Copy link
Author

I am not sure the signals as they are now is the best option. E.g. why the actual removing/adding to history file is done in gui_core/goutput.py? I am thinking the actual adding/removing should be done wherever it is requested, so adding in core/gconsole.py and deleting in browser.py. The signals then should be informing the action has already happened, e.g. cmdPrompt needs to updated, tree needs to be updated. Right now the signals are both requesting change in history file and informing the change is happening. I believe this would clarify it, but maybe I missed something.

You are right, I will change that.

@lindakarlovska lindakarlovska force-pushed the wxGUI_delete_history_entry branch 6 times, most recently from 1e11417 to c670734 Compare January 8, 2024 09:49
@lindakarlovska lindakarlovska force-pushed the wxGUI_delete_history_entry branch from c670734 to ea8f3ea Compare January 8, 2024 09:55
@lindakarlovska
Copy link
Author

lindakarlovska commented Jan 8, 2024

@tmszi, @petrasovaa I had problems with updating from upstream/main. I rebased on upstream/main, solved conflicts, and tested the PR (to this moment everything is alright), but when I pushed I took with me many unrelated commits from upstream/main and was unable to get rid of them.

Normally, I do the following to fix it but it did not work (After this procedure I still had the unrelated commits as part of the PR):

  1. restore the main from upstream
    git fetch upstream
    git checkout main
    git reset --hard upstream/main
    git push origin main --force

  2. Create a new branch from the restored main:
    git checkout main
    git checkout -b wxGUI-delete-history-entry-2

  3. Cherry pick commits from rebase:
    git cherry-pick ...
    Screenshot from 2024-01-08 12-00-39

  4. Replace the branch with the new branch
    git push origin --force --set-upstream wxGUI-delete-history-entry-2:wxGUI_delete_history_entry

So to deal with the problem of unrelated commits ASAP I finally decided to completely reset this PR and create another new PR OSGeo#3342 updated on upstream/main in GRASS/main. Sorry for that confusing step, I believe there are better solutions but this was the only solution that came into my mind to deal with the problem as soon as possible..

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