notepad-plus-plus / notepad-plus-plus

Notepad++ official repository
https://notepad-plus-plus.org/
Other
22.53k stars 4.55k forks source link

[Feature Request] Changed History: Go to Next/Prev block #12248

Closed Yaron10 closed 1 year ago

Yaron10 commented 1 year ago

"Changed History" margin was introduced in NPP 8.4.6. Two menu commands "Go to Next/Prev block" would be a useful addition to that new feature.

Thank you.

vinsworldcom commented 1 year ago

A proof of concept in PythonScript:

def gotoLine(line):
    editor.setVisiblePolicy(CARETPOLICY.JUMPS | CARETPOLICY.EVEN, 0)
    editor.ensureVisibleEnforcePolicy(line)
    editor.gotoLine(line)
    editor.setVisiblePolicy(CARETPOLICY.EVEN, 0)
    editor.ensureVisibleEnforcePolicy(line)

def gotoNextChange(self):
    line = -1
    pos = editor.getCurrentPos()
    searchStart = editor.lineFromPosition(pos)

    ### `markerNext` doesn't work on ChangeHistory:
    #   https://sourceforge.net/p/scintilla/bugs/2353/
    lastLine = editor.lineFromPosition(editor.getTextLength())
    consec = searchStart
    for ln in range(searchStart+1, lastLine+1):
        if editor.markerGet(ln) & self._mask:
            if ln == consec + 1:
                consec += 1
            else:
                line = ln
                break

    if line == -1:
        for ln in range(0, searchStart):
            if editor.markerGet(ln) & self._mask:
                if ln == consec + 1:
                    consec += 1
                else:
                    line = ln
                    break

    if line != -1:
        gotoLine(line)

def gotoPrevChange(self):
    line = -1
    pos = editor.getCurrentPos()
    searchStart = editor.lineFromPosition(pos)

    while True:
        line = editor.markerPrevious(searchStart - 1, self._mask)

        if line == -1:
            break

        if line == (searchStart - 1):
            searchStart -= 1
        else:
            break

    if line == -1:
        end = editor.getLineCount()
        line = editor.markerPrevious(end, self._mask)

    if line != -1:
        gotoLine(line)

Cheers.

Yaron10 commented 1 year ago

@vinsworldcom,

Thank you for the nice script. 👍 Alas, it won't work for me. :)

vinsworldcom commented 1 year ago

Alas, it won't work for me. :)

Sorry, it's a proof of concept - extracted from my larger working script. I can try to make is fully functional:

from Npp import editor, CARETPOLICY

class ChangeHistoryGoTo(object):
    def __init__(self):
        self._mask = (1 << 21) | (1 << 22) | (1 << 23) | (1 << 24)

    def _goto_line(self, line):
        editor.setVisiblePolicy(CARETPOLICY.JUMPS | CARETPOLICY.EVEN, 0)
        editor.ensureVisibleEnforcePolicy(line)
        editor.gotoLine(line)
        editor.setVisiblePolicy(CARETPOLICY.EVEN, 0)
        editor.ensureVisibleEnforcePolicy(line)

    def gotoNextChange(self):
        line = -1
        pos = editor.getCurrentPos()
        searchStart = editor.lineFromPosition(pos)

        ### `markerNext` doesn't work on ChangeHistory:
        #   https://sourceforge.net/p/scintilla/bugs/2353/
        lastLine = editor.lineFromPosition(editor.getTextLength())
        consec = searchStart
        for ln in range(searchStart+1, lastLine+1):
            if editor.markerGet(ln) & self._mask:
                if ln == consec + 1:
                    consec += 1
                else:
                    line = ln
                    break

        if line == -1:
            for ln in range(0, searchStart):
                if editor.markerGet(ln) & self._mask:
                    if ln == consec + 1:
                        consec += 1
                    else:
                        line = ln
                        break

        if line != -1:
            self._goto_line(line)

    def gotoPrevChange(self):
        line = -1
        pos = editor.getCurrentPos()
        searchStart = editor.lineFromPosition(pos)

        while True:
            line = editor.markerPrevious(searchStart - 1, self._mask)

            if line == -1:
                break

            if line == (searchStart - 1):
                searchStart -= 1
            else:
                break

        if line == -1:
            end = editor.getLineCount()
            line = editor.markerPrevious(end, self._mask)

        if line != -1:
            self._goto_line(line)

if __name__ == '__main__':
    chgt = ChangeHistoryGoTo()

Run from the PythonScript menu, then from the PythonScript console:

chgt.gotoNextChange()
chgt.gotoPrevChange()

Cheers.

Yaron10 commented 1 year ago

תמונה

I'm using PythonScript 2.

Thanks again. I appreciate that. I wouldn't like to bother you with that.

vinsworldcom commented 1 year ago

I'm using PythonScript 2.

A-ha, I'm using 3.0.14

Yaron10 commented 1 year ago

Well, I'll wait for the integration of the feature in NPP. And when I have time (and mood), I'll learn your script. :)

Thanks again.

vinsworldcom commented 1 year ago

And when I have time (and mood), I'll learn your script. :)

Try again, I removed the enum dependency.

Yaron10 commented 1 year ago

It works like a charm!
And you jump to the Next/Prev block. Very nice. 👍

Thank you.

vinsworldcom commented 1 year ago

And you jump to the Next/Prev block. Very nice. 👍

Yes, that's key, otherwise it'll stop at every change line and if there are 20 new lines inserted in a row, you just want to stop at the top (next) or bottom (previous) - not each and every one.

Cheers.

Yaron10 commented 1 year ago

@vinsworldcom,

Thanks again for the beautiful script. 👍

Slightly adapted for my personal use. :)

# Based on https://github.com/notepad-plus-plus/notepad-plus-plus/issues/12248#issuecomment-1258561261.

class ChangeHistoryGoTo():
    def __init__(self):
        self._mask = (1 << 21) | (1 << 22) | (1 << 23) | (1 << 24)
        self._currentLine = editor.lineFromPosition(editor.getCurrentPos())

    def _goto_line(self, line):
        if line != -1 and line != self._currentLine:
            editor.ensureVisibleEnforcePolicy(line)
            editor.gotoLine(line)
        else:
            import winsound
            winsound.MessageBeep()

#----------------#

    def gotoNextOrFirstChange(self, gotoFirst):
        line = -1
        searchStartLine = self._currentLine
        lastLine = editor.getLineCount()

        if not gotoFirst:
            for searchLine in range(self._currentLine, lastLine):   # Start from currentLine (not currentLine + 1) in case currentLine is not-changed and the next line IS changed.
                if editor.markerGet(searchLine) & self._mask:
                    if searchLine != searchStartLine:      # changed-line found in a different block.
                        line = searchLine
                        break
                    else:
                        searchStartLine += 1

        if line == -1:
            for searchLine in range(0, lastLine if gotoFirst else self._currentLine):     # Wrap around.
                if editor.markerGet(searchLine) & self._mask:
                    line = searchLine
                    break

        self._goto_line(line)

#----------------#

    def gotoPrevChange(self):
        line = -1
        searchStartLine = self._currentLine

        while True:
            line = editor.markerPrevious(searchStartLine, self._mask)
            if line == -1 or line != searchStartLine:   # "line == -1": no changed-line found. "line != searchStartLine": changed-line found in a different block.
                break
            else:
                searchStartLine -= 1

        if line == -1:
            line = editor.markerPrevious(editor.getLineCount(), self._mask)     # Wrap around.

        self._goto_line(line)

#-------------------------------------------------------------------------------#

changeHistory = ChangeHistoryGoTo()
if clickButton == 0:
    changeHistory.gotoNextOrFirstChange(False)      # Next.
elif clickButton == 1:
    changeHistory.gotoNextOrFirstChange(True)       # First.
else:
    changeHistory.gotoPrevChange()
donho commented 1 year ago

@Yaron10 Could you describe the behaviour of "Go to Next/Prev block"? Is it based on cursor's positions? Should it be in the same document?

Yaron10 commented 1 year ago

@donho,

Is it based on cursor's positions?

Yes, it is.

Should it be in the same document?

Yes, I think it should be per file.

And we should jump from block to block: If lines 10-12 are changed, and the caret is at line 10 - we shouldn't go to line 11 but to the next block if exists.

I've implemented it in C++. Please ignore my ::MessageBeep() calls. I find them helpful. :)

// Based on https://github.com/notepad-plus-plus/notepad-plus-plus/issues/12248#issuecomment-1258561261.
void Notepad_plus::changedHistoryGoTo(int idGoTo)
{
    int mask = (1 << 21) | (1 << 22) | (1 << 23) | (1 << 24);
    intptr_t line = -1;
    intptr_t blockIndicator = _pEditView->getCurrentLineNumber();
    intptr_t lastLine = _pEditView->execute(SCI_GETLINECOUNT);

    if (idGoTo != IDM_SEARCH_CHANGED_PREV)      // Next or First.
    {
        intptr_t currentLine = blockIndicator;

        if (idGoTo == IDM_SEARCH_CHANGED_NEXT)
        {
            // Start from currentLine (not currentLine + 1) in case currentLine is not-changed and the next line IS changed. lastLine is at least *1*.
            for (intptr_t i = currentLine; i < lastLine; i++)
                if (_pEditView->execute(SCI_MARKERGET, i) & mask)
                {
                    if (i != blockIndicator)        // Changed-line found in a different block.
                    {
                        line = i;
                        break;
                    }
                    else
                        blockIndicator++;
                }
        }

        if (line == -1)     // Wrap around.
        {
            intptr_t endRange = (idGoTo == IDM_SEARCH_CHANGED_NEXT) ? currentLine + 1 : lastLine;       //  "+ 1": currentLine might be *0*.
            for (intptr_t i = 0; i < endRange; i++)
                if (_pEditView->execute(SCI_MARKERGET, i) & mask)
                {
                    line = i;
                    if (idGoTo == IDM_SEARCH_CHANGED_NEXT)
                        ::MessageBeep(MB_OK);
                    else if (i == currentLine)
                        ::MessageBeep(MB_ICONINFORMATION);

                    break;
                }
        }
    }
    else    // Prev.
    {
        while (true)
        {
            line = _pEditView->execute(SCI_MARKERPREVIOUS, blockIndicator, mask);
            // "line == -1": no changed-line found. "line != blockIndicator": changed-line found in a different block.
            if (line == -1 || line != blockIndicator)
                break;
            else
                blockIndicator--;
        }

        if (line == -1) // Wrap around.
        {
            line = _pEditView->execute(SCI_MARKERPREVIOUS, lastLine - 1, mask);
            if (line != -1)
                ::MessageBeep(MB_OK);
        }
    }

    if (line != -1)
    {
        _pEditView->execute(SCI_ENSUREVISIBLEENFORCEPOLICY, line);
        _pEditView->execute(SCI_GOTOLINE, line);
    }
    else
        ::MessageBeep(MB_ICONEXCLAMATION);
}
donho commented 1 year ago

@Yaron10

Two menu commands "Go to Next/Prev block" would be a useful addition to that new feature.

I would say "Go to Previous Change" & "Go to Next Change" - what do you think?

I will integrate your code and do a PR - if you still don't want to do a PR.

Yaron10 commented 1 year ago

@donho,

If you're planing to place the commands directly under Search, "Go to Next/Prev Change" would be better. If the commands should be under a "Change History" popup (before or after "Bookmarks"?), "Go to Next/Prev Block" might be better. As you understand.

It's @vinsworldcom's code. I'd be glad if you could do the PR.

Thank you!

vinsworldcom commented 1 year ago

Sorry, away this weekend no computer access.

Sounds like the example would be from https://github.com/vinsworldcom/nppChangedLines

Cheers.

On Sun, Jul 2, 2023, 4:50 PM Yaron10 @.***> wrote:

@donho https://github.com/donho,

If you're planing to place the commands directly under Search, "Go to Next/Prev Change" would be better. If the commands should be under a "Change History" popup (before or after "Bookmarks"?), "Go to Next/Prev Block" might be better. As you understand.

It's @vinsworldcom https://github.com/vinsworldcom's code. I'd be glad if you could do the PR.

Thank you!

— Reply to this email directly, view it on GitHub https://github.com/notepad-plus-plus/notepad-plus-plus/issues/12248#issuecomment-1616814180, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACJVWUMLJYUL336BKHSZRBDXOHNJ7ANCNFSM6AAAAAAQWDFD4M . You are receiving this because you were mentioned.Message ID: @.***>

donho commented 1 year ago

Sorry, away this weekend no computer access. Sounds like the example would be from https://github.com/vinsworldcom/nppChangedLines

@vinsworldcom Nice plugin! Is there any reason for Changed Lines plugin not being included in NppPluginList ?

vinsworldcom commented 1 year ago

Most of the original code was from before changed. History was included in notepad Plus Plus and I "borrowed" it from location navigate plugin.

On Sun, Jul 2, 2023, 6:21 PM Don HO @.***> wrote:

Sorry, away this weekend no computer access. Sounds like the example would be from https://github.com/vinsworldcom/nppChangedLines

@vinsworldcom https://github.com/vinsworldcom Nice plugin! Is there any reason for Changed Lines plugin not being included in NppPluginList ?

— Reply to this email directly, view it on GitHub https://github.com/notepad-plus-plus/notepad-plus-plus/issues/12248#issuecomment-1616867149, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACJVWUIJKHDXE4RNJ3AKIQTXOHX7DANCNFSM6AAAAAAQWDFD4M . You are receiving this because you were mentioned.Message ID: @.***>

donho commented 1 year ago

@vinsworldcom

I think it's still worth to add it into plugin admin. Do you mind to add it?

alankilborn commented 1 year ago

The Changed Lines plugin predates "Change History" in Scintilla/Notepad++. Now that change history is here, the plugin offers too much redundancy and should be retired. I believe @vinsworldcom agrees. Spreading the word about the plugin, i.e., offering via Plugins Admin is IMO a bad idea.

donho commented 1 year ago

@vinsworldcom @alankilborn OK, I see your point. Though I think it's still interesting for users to have a list of their modification content. Also users can install/uninstall if they like/dislike this feature - that's the whole point of plugin. Anyway, NppPluginList is opened for all plugin authors, and it's the decision of plugin authors to make their plugins available or not in Plugin Admin.

Yaron10 commented 1 year ago

There's a nice "Clear Change History" command in @vinsworldcom's plugin. I'm wondering if it can be easily implemented in NPP.

alankilborn commented 1 year ago

interesting for users to have a list of their modification content

Then this would be a good idea for a feature within Notepad++ itself. :-)

donho commented 1 year ago

@Yaron10

I'm wondering if it can be easily implemented in NPP.

If such feature can be in Change Lines plugin, it can be built-in feature in Notepad++. The question is, where should we put it?

Yaron10 commented 1 year ago

@donho,

If such feature can be in Change Lines plugin, it can be built-in feature in Notepad++.

Great. 👍

The question is, where should we put it?

How about putting it in a "Change History" popup under Search? "Go to Next Change". "Go to Previous Change". Separator. "Clear History of the Current Document".

Thank you.

Yaron10 commented 1 year ago

Another question: Should those commands be disabled if the History margin is not displayed? Or just doing nothing (or beep :) ) would be enough?

donho commented 1 year ago

Should those commands be disabled if the History margin is not displayed? Or just doing nothing (or beep :) ) would be enough?

I'll see what I can do about it.

alankilborn commented 1 year ago

How about putting it in a "Change History" popup under Search?

popup = submenu

Should those commands be disabled if the History margin is not displayed?

Disabled makes good sense. Beep is OK, but disabled is better IMO.

Yaron10 commented 1 year ago

@donho,

I'll see what I can do about it.

👍 Thank you.

I just want to point out that the Bookmarks commands are enabled even if the margin is not displayed. So returning and doing nothing is also an option. - Not as good as disabled though.

@alankilborn,

Beep is OK, but disabled is better IMO.

I was joking. I knew you and Don wouldn't like the beep.

alankilborn commented 1 year ago

Bookmarks commands are enabled even if the margin is not displayed

Are some new issues going in? :-)

I'd say that the proposed new features should be done correctly from the start, i.e., don't use existing bad behavior as something to align with.

Yaron10 commented 1 year ago

@alankilborn,

I'd say that the proposed new features should be done correctly from the start

👍

Actually my analogy was completely wrong: you can create and go-to bookmarks even if the margin is not displayed.


"Clear History of the Current Document" or "Clear Current Document History"?

vinsworldcom commented 1 year ago

Clear Change History

Just be aware that to do that, you lose your undo buffer.

vinsworldcom commented 1 year ago

The Changed Lines plugin predates "Change History" in Scintilla/Notepad++. Now that change history is here, the plugin offers too much redundancy and should be retired. I believe @vinsworldcom agrees.

The plugin does a little few things beyond the existing Notepad++ support for Scintilla Change History - that's why I kept it. I did strip out the entire change tracking that was implemented. The "extras" are:

All of this is described in the README. Most could be done with PythonScripts. Not sure how much appetite there is for porting any of this "natively" to Notepad++. I like the extra features and use them so will continue to use the plugin as more of an add-on to existing capability (something I usually do with PythonScripts) vs. adding a brand new unique capability - as this did before Scintilla Change History.

As I said, the original code was "borrowed" from Location Navigate plugin, but since change tracking is out of the plugin now - most of the remaining code I wrote and you can do with it what you like. Happy to see some of all of this move to Notepad++.

Cheers.

donho commented 1 year ago

Thanks to Change Lines plugin's feature Clear Change History, the bug regarding Reload from Disk (#12319, #12261 & #13735) is fixed now. For people who are interested in it, check PR #13858