Closed Chris2011 closed 5 years ago
Did this need a second PR? Could it not have been done in the first one?
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.
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:
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?
Can one of the admins verify this patch?
Smth missing here for the merge?
@Chris2011 I don't see any new commits based on my comment from Sep 23rd.
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?
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
?
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(?)
But if you instantiate the action manually with new SplitDocumentVerticallyAction
how is the shortcut set? Maybe I'm missing something really obvious.
The shortcut is set here: https://github.com/apache/incubator-netbeans/pull/1/commits/67bdd60109474c543c010d3f818cfb36abb9c0c9#r150193869
The functionality is still there, if you right click on an editor tab -> Split -> Vertically. So I decoupled the functionality from https://github.com/apache/incubator-netbeans/pull/3/commits/5dc272ebffd95dfd55720ebfdd697d8ff319c862#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.
Please see my latest commits, I splitted my one commit into two as requested. Thx.
Can someone have a look now, please? :)
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.
@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.
@sdedic thx for reviewing, I will have a look later.
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.
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.
@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
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?
@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.
Would be good for @sdedic and @emilianbold to take a quick look.
Looking at it.
To me this looks like a very clean patch of what this PR is all about: https://github.com/Chris2011/incubator-netbeans/compare/Chris2011:master...emilianbold:feature/248233-doc-split-actions?expand=1 (I've also made a PR against Chris' repository https://github.com/Chris2011/incubator-netbeans/pull/1 ): you just want to register some actions which have some shortcuts and call splitWindow / clearSpit.
Is anything missing if we do it this way?
Any reason SplitAction had to be changed to use the same SplitDocumentHorizontallyAction, etc. other than code 'purity'?
I guess the PR got better with the separate commits but I don't find it good because it changes too much for no reason. Why introduce that tiny abstract class just to store that int orientation
? Also having that initTopComponent method and the separate action constructors that accepts a TopComponent seemed odd somehow.
If my changes are OK and work you have my vote for it and are free to merge it.
I think it's good; thanks, @Chris2011
PS: Note that SplitAction is registered in the layer and visible under the View menu. Another approach since you create official actions for each would be for that registration to be a normal sub-folder containing your actions... :-)
The reason for changing was, I changed the code anyway so why not doing a bit of refactoring like decouple them into separate action classes? Sure, the code of NB is huge but why not doing it a bit better than before? The tutorials of creating actions with 3rd-party-plugins, works exactly that way. Create an action class, add the actionreferences, etc.
I thought it could be good to clean up the code a bit, sure maybe it was more code than before but it is more maintainable. It was good to start to refactor the code to bring in consistency. Actions are in action folders (If they make sense). Actions are in separate classes (One class does exactly one thing, SplitHo., SplitVer, ClearSplit, etc.)
I added an Actions folder before but the problem was discussed some comments above, trouble with the translations etc. I've totally forgotten it.
@emilianbold sure your approach should work too. As I said, I wanted to refactor the code a bit too.
But I'm also fine with your code.
@geertjanw what is the preferred way, doing the stuff like @emilianbold or is my good enough? I don't know the answer. Sure, I'm fine with it to change my code to emilians PR and push it again.
Well, I was just trying to understand what you want to accomplish. My patch seems much shorter and does the same (or not?). I'm not certain what your refactoring adds -- it anything it introduces complexity.
But, if I were to refactor, I'd take into account that SplitAction
is registered in mf-layer.xml
under Menu/View
.
Ideally, since we have standalone actions now for each horizontal split / vertical split / clear action then we should create
<folder name="Menu">
<folder name="View">
<folder name="Split">
<attr bundlevalue="org.netbeans.core.multiview.Bundle#CTL_SplitAction" name="displayName"/>
</folder>
and register your SplitDocumentHorizontallyAction
and SplitDocumentVerticallyAction
with an @ActionReference
under path = "Menu/View/Split"
.
Then you have to take into account that SplitAction
seems to have a toggle via isSplitingEnabled
and that the actions are enabled/disabled based on the activated TopComponent.
Of course, what's missing with using the layer is that you cannot disable the whole View/Split
menu if it's a layer folder.
So, as far as refactoring go it seems you stopped too early. In which case it seems better not to refactor anything and keep the patch more minimal.
Anyhow, I would like to conclude with this PR. It seems to have taken forever.
I also don't want to be nitpicking and stop progress. So, I'm going to unsubscribe from this PR. Feel free to either continue working on the patch based on my comments or your own ideas or commit it as is, etc.
@emilianbold ok I understand your point. Maybe I not finished my thinking about the refactoring part. Anyway, I'm fine with merging your PR into my branch and will push it again. Thx for anything :)
So now I tested and merged the changes from @emilianbold PR to my PR. Hope now everything is fine.
So I will wait until next friday and will merge, if it is allowed(?) Please leave a last comment thx :)
Sure, happy to merge this, if @emilianbold gives his final OK, since he's worked with you a lot on this.
:+1:
Hm, maybe there was smth wrong with the merge :/. Sry about that. Will change and test it later. Thx @emilianbold
I fixed most of the requested changes now.
@emilianbold can you check it again please?
Take a look at this diff https://github.com/apache/incubator-netbeans/compare/master...emilianbold:feature/248233-doc-split-actions and compare with this PR's changes https://github.com/apache/incubator-netbeans/pull/3/files
@Chris2011 Maybe, Emi says it is better to add only the necessary changes. e.g. You added some new lines. BTW, you should use /*
as the first line of license header instead of /**
.
I thought I only added that, what emi suggested. So after this, the merge was kind of "broken" so I tried to fix it manually.
But in my opinion, this shouldn't be so bad to merge it now. It is as it is, but the concept was to make is simple and only do, what needed. And this was done now.
But in my opinion, this shouldn't be so bad to merge it now. It is as it is, but the concept was to make is simple and only do, what needed. And this was done now.
I personally prefer patches to be tight and not reformat code, or add random lines or whitespace, or fix unrelated bugs, etc.
The PR is not "so bad" to be un-mergeable right not but you are also 99% done and instead of posting that comment you could have amended the commit and be done with it.
@emilianbold thx for your opinion and I really know what you mean. I will be more strict in the next PRs and will keep your opinion in mind and everything, what you said. I really can understand what you mean. I will merge the PR now, if there are no other big problems from whoever. Thx for the great discussion.
I'm not sure why you don't change that... NOTE: You should not merge this using "Merge pull request" button as it is. This PR includes many irrelevant commits.
I suppose you should fix the license header although it is a trivial thing.
@matthiasblaesing Could you help this? I think that Chris should rebase and squash this PR in Chris's local machine or someone should merge it using "Squash and merge" button.
@junichi11 Sry, I missed the point with the header. Will fix it. Thx. Yeah maybe I need some help with that, what is not working out. I rebased, and my stuff is on top of it.
I tried a classic git rebase - I'll make it short: I aborted it, as it ended with a lot of merge conflicts. The history of this PR is long and winding and this makes it hard.
In this case I would try the github "Squash and merge" variant. This should result in one commit in the history.
You could do it by hand:
pr3-merged
github/pr/3
which holds the changes from this PRgithub/pr/3
into pr3-merged
master
and pr3-merged
: git diff master..pr3-merged > ../pr3.patch
the output can be outputted into a file and that patch is the core of this PRmaster
feature/248233-doc-split-actions
branch locallyfeature/248233-doc-split-actions
git apply ../pr3.patch
feature/248233-doc-split-actions
in the repository Chris2011
If you have done it before, this can be done quickly, if not, I would not start in a big repository like netbeans :-)
@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.
:+1: 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:
git branch -m feature/248233-doc-split-actions-old
Cleaned up code as discussed here: #1
Everything fine now?