NETBEANS-59 - Document split actions

[Chris2011]

Cleaned up code as discussed here: #1

Everything fine now?

[mcdonnell-john]

Did this need a second PR? Could it not have been done in the first one?

[Chris2011]

Maybe not, but I reverted everything, cloned the repo new, pulled from the original master, created a new branch, copied over the files manually and change the line endings.

[emilianbold]

Of course, it's much easier to read since there are no more newline changes.

What this patch seems to do is add an @ActionReference so we can have shortcuts for those 3 actions.

So, as a fix I would have preferred 2 commits:

• one fixing in-place the existing actions (in order to see the actual meat) and
• another commit refactoring the classes.

I seems the existing static inner classes might have been sufficient: you could have just made them public. You could have introduced SplitDocumentHorizontallyAction which just extends SplitDocumentAction and calls super(tc, JSplitPane.HORIZONTAL_SPLIT). Same for SplitDocumentVerticallyAction.

I believe initTopComponent only exists so you can have a no-arg constructor which is odd because you are instantiating the actions manually (so it must have to do with the registrations). Also, you know that (tc instanceof Splitable && ((Splitable)tc).canSplit()) before it's called.

So, what I am curious to learn is how does the shortcut work for your manually created instance?

[asfgit]

Can one of the admins verify this patch?

[Chris2011]

Smth missing here for the merge?

[emilianbold]

@Chris2011 I don't see any new commits based on my comment from Sep 23rd.

[Chris2011]

Most of the code, was only moved out to separated the code and created actions, I only created one base class by my self. You only mean that I should split it up to 2 commits?

The last sentence I don't understand in your comment:

So, what I am curious to learn is how does the shortcut work for your manually created instance?

[emilianbold]

I wrote a rather long message in September. Does nothing I said there make sense?

It hard to review a patch that has +320 line and −101 lines in a single commit.

From what I remember and what I deduced while trying to figure it out, you just added an @actionreference and some helper class then refactored.

Reviewing would be much simpler if refactoring would be a separate commit.

The last sentence I don't understand in your comment:

So, what I am curious to learn is how does the shortcut work for your manually created instance?

I was just curious how the solution even works considering the action is created manually and not by the Platform. How does the shortcut plumbing work? Because, maybe you don't even need an @actionreference?

[Chris2011]

It hard to review a patch that has +320 line and −101 lines in a single commit.

This is what I didn't get yet, I didn't get your point of this. Thx it is clearer.

Yes, I added 3 ActionReferences for 3 new actions "Split Action Horizontally", "Split Action Vertically" and "Clear Split" because those actions didn't exist. Now you can change the hotkeys for each of them, which was not possible before.

I only set such ActionReferences to have Actions, where I can add shortcuts to it. It is from the official Documentation I think, how we can create actions. I don't know an other way. Only the layer.xml but this is obsoleted I think(?)

[emilianbold]

But if you instantiate the action manually with new SplitDocumentVerticallyAction how is the shortcut set? Maybe I'm missing something really obvious.

[Chris2011]

The shortcut is set here: 67bdd60#r150193869

The functionality is still there, if you right click on an editor tab -> Split -> Vertically. So I decoupled the functionality from 5dc272e#diff-c2b5538f4bdcf56983c605aebf73887a and created new classes for each action to set the ActionReference.

Now you can use the functionality like before via Right click on editor tab -> Split -> Vertically OR you can use the shortcut, which I set to Ctrl + Alt + Shift + V or you can set your own.

[Chris2011]

Please see my latest commits, I splitted my one commit into two as requested. Thx.

[Chris2011]

Can someone have a look now, please? :)

[ghost]

Hello @Chris2011,

I believe what @emilianbold meant by having two commits is that the first commit would implement the requested feature while keeping the SplitDocumentAction and ClearSplitAction nested classes within org.netbeans.core.multiview.SplitAction. Presumably you would need to add SplitDocumentHorizontallyAction and SplitDocumentVerticallyAction nested classes so that you could add the specific @ActionID, @ActionRegistration, and @ActionReference annotations. Then, once you have done this and verified that you are able to invoke the actions via the keyboard shortcuts, the second commit would be a simple refactoring that would move the ClearSplitAction, SplitDocumentAction, SplitDocumentHorizontallyAction, and SplitDocumentVerticallyAction nested classes to top-level classes within the org.netbeans.core.multiview.actions package.

One subtle issue is that moving message strings LBL_ClearSplitAction, LBL_ValueClearSplit, LBL_SplitDocumentActionHorizontal, LBL_ValueSplitHorizontal, LBL_SplitDocumentActionVertical, LBL_ValueSplitVertical to a different package (from org.netbeans.core.multiview to org.netbeans.core.multiview.actions) will break existing translations of these strings. For example, there are Czech translations at https://netbeans.org/projects/nblocalization/downloads/directory/8.0.2 which would no longer work. Maybe these message strings could be kept within the org.netbeans.core.multiview package and the relocated ClearSplitAction, etc. could refer to org.netbeans.core.multiview.Bundle for these strings. Or, maybe it's fine to move the strings to the different package. I am not sure.

[sdedic]

Maybe better check not for tc != null but tc instanceof Splitable, which covers bot non-null AND the required interface presence. Maybe even the check for getSplitOrientation could move into clearSplit

[sdedic]

Consider category "Windows" or "Windows / Layout" rather than Tools.

[sdedic]

Could be the orientation set from the constructor (it is constant).

[sdedic]

Also consider creating @ActionReference for placement in Menu/Windows. Note that the registered singletons (unlike instance created in window popup menus) do not update their enable flag according to the current TC.

[Chris2011]

@dtrebbien thx for the explanation, the refactoring was needed to added the ActionIds, I tried it before I refactored the code, because that wasn't my intention. So I changed the code a bit and got it working, while I created new actions in external classes. Maybe I made a mistake and it would work right inside as nested classes but now I think it is a bit cleaner.

[Chris2011]

@sdedic thx for reviewing, I will have a look later.

[emilianbold]

BTW, regarding my remark:

But if you instantiate the action manually with new SplitDocumentVerticallyAction how is the shortcut set? Maybe I'm missing something really obvious.

I believe there may be 2 instances of the same action. One added manually with new which basically does not have a keyboard shortcut and another one added by the NetBeans infra which does have the binding. My confusion was probably assuming there is a single instance.

I would appreciate somebody confirming this or not, etc. because it was not obvious to me.

[Chris2011]

Yeah so the actions, which are added manually via new are for the contextmenu of an opened tab

and for the menu

Window -> Configure Window -> Split Document.

Those actions where added before now I only create a new instance, but yes maybe it could be done even better. Changes are appreciated too.

[Chris2011]

@sdedic markiewb and me had a coding session yesterday. Big thx to him. He helped me a lot with other stuff and with this PR. So in general we fixed everything what was requested (Refactoring, simplifications, etc.). Hope this is now enough to merge the request.

We tested all actions. View -> Split; Window -> Configure Window -> Split Document; Right Click on an editor tab -> Split; And my shortcuts.

@emilianbold Yes those are 2 instances. One for the menu actions and one for my shortcuts via the ActionReference. But both of us, don't see any problem with this. We added some comments too to describe the constructors a bit better.

Cheers

Chris

[geertjanw]

Great. So @Chris2011, from your and markiewb point of view, everything is now done and there's nothing at all that should block this one from being accepted?

[Chris2011]

@geertjanw right. We don't see any other problems with that stuff. Maybe an other can test it before, if someone wants it. But from our pov, everything should be fine.

[junichi11]

@matthiasblaesing Thanks a lot for your help!

In this case I would try the github "Squash and merge" variant. This should result in one commit in the history.

👍
Could you merge this PR if it's OK with Chris?

BTW, diff of this PR is here: https://github.com/apache/incubator-netbeans/pull/3.diff
So, in this case, Chris might be able to use the following steps:

• Download the diff file as a patch
• Rename the branch: e.g. git branch -m feature/248233-doc-split-actions-old
• Checkout master (see Matthias's comment)
• Apply the patch (see Matthias's comment)
• Force push (see Matthias's comment. Especially, please note "in the repository Chris2011".)

[matthiasblaesing]

Chris is a committer, so he can commit himself. @Chris2011 if you need help, feel free to contact me.

[junichi11]

Sure. Anyway, I wanted to avoid merging it as it is :)

[Chris2011]

Will try that. Thx.

[Chris2011]

So now, everyhing should be fine now. I renamed the old branch, made a clean one, updated all branches that is needed, applied the patch, changed the line endings from CRLF to Unix LF. 3 files are new, which are the 3 actions (ClearSplit, SplitDocumentVerticallyAction and SplitDocumentHorizontallyAction). The SplitAction.java file only removes some Bundle messages.

Atm, I don't know why is the SplitAction completely red and green again but finally, it shouldn't matter now. Everything was discussed earlier. Please merge now.

[matthiasblaesing]

Please recheck SplitAction - it now has windows line endings (\r\n), which is also the reason, that it looks so red.

[Chris2011]

Hm, I closed the file and reopened it, now it is CRLF, will fix it.

[Chris2011]

It doesn't make sense to change it now to LF and amend the commit, right? Do I have to do it again? I mean applying the patch and fix the line endings?

[matthiasblaesing]

If you change line endings, do an amend commit and then force push that should do it.

[Chris2011]

Got it.

[matthiasblaesing]

You now lost the commit message :-) - When you amend you need to keep in mind, that you replace the previous commit.

[Chris2011]

Hm, ok. I thoght they will merged together. The Message too. But this should Not be a Problem now. Now I know it and will make it better next time :).

[matthiasblaesing]

Why wait? An amend commit is possible without any file changes, so you can still fix the commit message. Same procedure s before: Start the amend commit, check/update the message, finish the commit, force push. Am 15. September 2018 23:26:34 MESZ schrieb Chrizzly <notifications@github.com>:

…

Hm, ok. I thoght they will merged together. The Message too. But this should Not be a Problem now. Now I know it and will make it better next time :).

[Chris2011]

Done.

[matthiasblaesing]

Looks better, though I would not choose this message. I would have worded it:

[NETBEANS-59] Separate actions for document splitting to enable shortcuts

<Context explaining why it was implemented the way it was>

It is a summary what was the desired outcome of this commit, which helps to understand the code changes. If an explanation is required why it was implemented the way it was or context should be provided, the further lines of the message can be used. The context is not always required, but helps to put the understand changes years later.

You can change the message or merge to master as is.

[Chris2011]

I can understand your point but first, my commit message is exact that, what the commit means. Maybe it is only a wording for sure and I know that, because I do it every day as all of you, But to be clear, the commit message should be as short as possible to explain what the commit makes. Here we only have one commit, but often we have more.

Second, everything else, what is not kind of related to the commit, is written in this ticket: https://issues.apache.org/jira/browse/NETBEANS-59. There is the exact explanation why it was implemented or why I want it or what I want. So If you need further information, look into the ticket.

Thx to all of us with my first apache netbeans PR for the info, help, hints and everything. Next time, I will be more careful. But please don't be that strict to everyone. I think a lot of people can lose motivation. IMHO. Will merge it now.

[matthiasblaesing]

I often use the "Show annotations" functions when working with code bases. Often the commit message holds the information that is necessary to understand why something was done the way it was. I see good commit messages in the same league as good comments: I'm not interested in comments, that just repeat what is obvious, but that give the code some meaning/context (explain the why). In the same sense good commit message give context (summary of the change) and explain why the change was done.

I have yet to see an instance where I took the time to wander through several issue reports, even if they are referenced. Switching to the issue tracker takes time and often issue texts don't help or the real explanation is buried in one of the uncounted number of comments.

Consider the current situation: Oracle could shutdown the bugzilla instance of netbeans right now. If the commit messages are meaningfull and explain the main reasoning, history could be reconstructed from my mercurial clone (or the git conversion of that). The commit history in a DVCS is by definition a good place to give context information.

Don't take this wrong: I appreciate your work and also the stamina to see this through.

I will keep being strict about these things. I have seen code bases, where commits looked like shit and the earlier a contributer learns about good commits, the better for him/her and the project and the commiter.