Notes feature added to Site Tree Node

[amitpanwar789]

Overview

Added Notes feature in Site Tree

Related Issues

Related to: #8240

Checklist

• Update help
• [] Update changelog
• Run ./gradlew spotlessApply for code formatting
• Write tests
• Check code coverage
• Sign-off commits
• Squash commits
• Use a descriptive title

For more details, please refer to the developer rules and guidelines.

[ganesh-dagadi]

Please squash the commits
https://www.freecodecamp.org/news/git-squash-commits/

[kingthorin]

The history IDs should probably be sorted, and the text input only active/enabled when an ID is selected (you could default select the first one I guess). Otherwise I'm sure entering text and not having one selected will fail somehow 😉
(This feedback based solely on the screenshot)

[kingthorin]

@thc202 this should be checking whether or not the HistoryReference(s) have a temporary type? (No point trying to put notes on those, right?)

HistoryReference.getTemporaryTypes()

[amitpanwar789]

@thc202 this should be checking whether or not the HistoryReference(s) have a temporary type? (No point trying to put notes on those, right?)

HistoryReference.getTemporaryTypes()

can you please tell me what is temporary type and how a user change type of href ?

[kingthorin]

Users can't aren't meant to change them, they're set programmatically. There are temporary nodes in the tree to facilitate display, but they're removed when the session ends.

[thc202]

#8307 (comment)

Correct.

[amitpanwar789]

Users can't aren't meant to change them, they're set programmatically. There are temporary nodes in the tree to facilitate display, but they're removed when the session ends.

ok , i will do it, but HistoryReference.getTemporaryTypes() return a set of integers and how can i check from this set if a href is TemporaryType or not ?

[kingthorin]

On the reference you can getHistoryType() the type and check if it's in the collection.

[amitpanwar789]

On the reference you can getHistoryType() the type and check if it's in the collection.

@kingthorin i think this will work

   private void fillJSet(Set<HistoryReference> set, SiteNode curSiteNode) {
        HistoryReference curHref = curSiteNode.getHistoryReference();
        if(!HistoryReference.getTemporaryTypes().contains(curHref.getHistoryType())) {
            set.add(curHref);
        }
        
        Vector<HistoryReference> pastHistoryReference = curSiteNode.getPastHistoryReference();
        for(HistoryReference href : pastHistoryReference) {
            if(!HistoryReference.getTemporaryTypes().contains(href.getHistoryType())) {
                set.add(href);
            }
        }
        

        for (Object o : Collections.list(curSiteNode.children())) {
            fillJSet(set, (SiteNode) o);
        }
    }

[kingthorin]

LGTM

[kingthorin]

That's the last thing I see. I still haven't pulled the PR and tested it, but it seems okay.

[kingthorin]

Thanks

[amitpanwar789]

Hi @thc202 , Any chance you could peek at my PR for any feedback ? Eager to jump on next issue.

[thc202]

Sorry but this didn't (and doesn't) seem to be ready yet:

• The note icon is not being shown in the Sites tree. Allow to add notes to Sites Nodes #8240 (comment).
• No warn when deleting history entries with notes. Allow to add notes to Sites Nodes #8240 (comment)

Edit: To be clear, it's fine if you don't want to implement those, but you should mention that.

The help should be being updated too.

[thc202]

new PopupMenuSiteNote(getView()))

[amitpanwar789]

new PopupMenuSiteNote(getView()))

why we would pass getView to PopupMenuSiteNote it need extensionHistory as parameter ? i have take the reference from PopMenuNote.

[thc202]

Because it's the only dependency it has, the dialog can live in the pop up menu item (less API exposure overall, this should be moved to an add-on at some point).

[amitpanwar789]

sorry but still didn't get it.

[kingthorin]

I think he is suggesting the only reason it passes the extension is for its view.

[thc202]

Instantiate it directly, as mentioned in the first comment.

[amitpanwar789]

done

[thc202]

The unused popupMenuSiteNote instance variable can be removed. The ZAP comment should be updated accordingly to latest changes.

[amitpanwar789]

do you mean the comments in ExtensionHistory file ? i think the previous comment is still appropriate.
// ZAP: 2024/01/25 Added SiteNotesAddDialog and its utility methods (Issue 8240).

[thc202]

It's just adding the popup menu item now.

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[amitpanwar789]

I have read the CLA Document and I hereby sign the CLA

[amitpanwar789]

Sorry but this didn't (and doesn't) seem to be ready yet:

• The note icon is not being shown in the Sites tree. Allow to add notes to Sites Nodes #8240 (comment).
• No warn when deleting history entries with notes. Allow to add notes to Sites Nodes #8240 (comment)

Edit: To be clear, it's fine if you don't want to implement those, but you should mention that.

The help should be being updated too.

for the second part do we need to warn the user if he is deleting siteNode or if he is deleting Href from historyExtension list window ?

[thc202]

From my understanding, the latter.

[amitpanwar789]

From my understanding, the latter.

I think for this we can just edit the existing warning from ExtensionHistory.java text like
"Are you sure you want to delete the record(s)?
This will also delete if any Note is associated with this record(s). "

[thc202]

I'd prefer if it was added only when there are notes but that should be fine too.

[amitpanwar789]

Sorry but this didn't (and doesn't) seem to be ready yet:

• The note icon is not being shown in the Sites tree. Allow to add notes to Sites Nodes #8240 (comment).
• No warn when deleting history entries with notes. Allow to add notes to Sites Nodes #8240 (comment)

Edit: To be clear, it's fine if you don't want to implement those, but you should mention that.

The help should be being updated too.

where do i need to update help section ?

[amitpanwar789]

Sorry but this didn't (and doesn't) seem to be ready yet:

• The note icon is not being shown in the Sites tree. Allow to add notes to Sites Nodes #8240 (comment).
• No warn when deleting history entries with notes. Allow to add notes to Sites Nodes #8240 (comment)

Edit: To be clear, it's fine if you don't want to implement those, but you should mention that.

The help should be being updated too.

where do i need to update help section ?

??

[kingthorin]

In the add-on, you can model it similar to others, just look at the src tree.

As for dealing with help for the core component that'll be done in zap-core-help. But is separate so don't get stuck on it for now.

[amitpanwar789]

@thc202 Is this complete or do I need to implement anything else ?