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

[NETBEANS-1979] Create Mode from client code #1135

Merged
merged 28 commits into from May 25, 2019

Conversation

@phipma
Copy link
Contributor

commented Feb 13, 2019

I have backed out the incompatible API changes and added a ModeUtility class following the advice from @JaroslavTulach. I hope this is better...?

Kind regards
Mark Phipps

phipma added some commits Oct 31, 2018

Allow any kind of Mode to be created programatically from a ModeConfig
XML String so that a TopComponent may then be programatically docked
into that Mode.
[NETBEANS-1979] Allow Modes to be created programatically.
Allow any kind of Mode to be created programatically from a ModeConfig
XML String so that a TopComponent may then be programatically docked
into that Mode.

Mark Phipps. mwgphipps@gmail.com mark.phipps@sucfin.com
Merge pull request #2 from apache/master
Merge remote-tracking branch 'upstream/master'
[NETBEANS-1979] Backout changes to original Mode api in favour of ext…
…ending that interface and providing utility helper class.
[NETBEANS-1979] Backout changes to original WindowManager api in favo…
…ur of extending that interface and providing utility helper class.
@JaroslavTulach
Copy link
Contributor

left a comment

I think the changes are OK in general. Consider the few suggested changes, increase specification version of the module in manifest.mf, add @since tag to refer to that version, and also write an entry into apichanges.xml to with references to the new API types.



/** This class is an implementation of Mode interface.
* It designates 'place' on screen, at wich TopComponent can occure.
*
* @author Peter Zavadsky
*/
public final class ModeImpl implements Mode {
public final class ModeImpl implements Mode.Xml {

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Feb 21, 2019

Contributor

OK, Mode.Xml sounds like a good way to extend the existing interface.

@@ -48,7 +50,7 @@
* @author Peter Zavadsky
*/
@org.openide.util.lookup.ServiceProvider(service=org.openide.windows.WindowManager.class)
public final class WindowManagerImpl extends WindowManager implements Workspace {
public final class WindowManagerImpl extends WindowManager implements Workspace, WindowManager.ModeManager {

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Feb 21, 2019

Contributor

Unlike Mode, WindowManager is abstract class - e.g. the methods could be added to it. They would just need some default implementation.

This comment has been minimized.

Copy link
@phipma

phipma Mar 18, 2019

Author Contributor

Moving methods back into WindowManager from ModeUtilities.

@@ -1364,6 +1363,16 @@ public static String escapeTcId4XmlContent (String tcName) {
}
return tcName;
}

public ModeConfig createModeFromXml(String xml) throws IOException {
ModeParser modeParser = ModeParser.parseFromString("NO_NAME_YET", new HashSet(1));

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Feb 21, 2019

Contributor

We used to add // NOI18N to mark strings that shouldn't be translated. Having them in some static final String constant would be even better, especially when used sever times.

new HashSet(1) seems strange. Such size cannot even access a single element (as the ratio is 0.75 by default and 1 * 0.75 < 1), is that what you wanted?

This comment has been minimized.

Copy link
@phipma

phipma Mar 15, 2019

Author Contributor

I copied HashSet(1) from the original code in WindowManagerParser:

    /**
     * Loads Mode configuration from the given file.
     * @param fo
     * @return
     * @throws IOException 
     * @since 2.70
     */
    public static ModeConfig loadModeConfigFrom( FileObject fo ) throws IOException {
        String modeName = fo.getName();
        ModeParser parser = new ModeParser(modeName, new HashSet(1));
        parser.setInLocalFolder(true);
        parser.setLocalParentFolder(fo.getParent());
        return parser.load();
    }

That was the only occurrence of calling that ctor without a Set of TopComponent names I think...

This comment has been minimized.

Copy link
@phipma

phipma Mar 15, 2019

Author Contributor

Changing to static final and default HashSet ctor.

*
* @author Mark Phipps
*/
public class ModeManagerTest extends NbTestCase {

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Feb 21, 2019

Contributor

OK, this looks like a test that tests something.

This comment has been minimized.

Copy link
@phipma

phipma Mar 18, 2019

Author Contributor

Changed name to WindowManagerModeTest.

// Remove unwanted Modes.
for (Mode mode: wm.getModes()) {
if (mode.getName().startsWith("anonymous")) {
ModeUtility.removeMode(wm, mode);

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Feb 21, 2019

Contributor

ModeUtility!?

This comment has been minimized.

Copy link
@phipma

phipma Mar 18, 2019

Author Contributor

This will just be wm.removeMode(mode);

* @see http://wiki.apidesign.org/wiki/ExtendingInterfaces
*
* @author Mark Phipps
*/

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Feb 21, 2019

Contributor

If we want to go this path of static utility helper method class, then I'd suggest to use ModeUtilities name. And again @since tag.

This comment has been minimized.

Copy link
@phipma

phipma Mar 15, 2019

Author Contributor

Renaming to ModeUtilities.

* @param windowManager typically WindowManager.getDefault()
* @param xml the XML that was originally produced by {@link #toXml}
*/
public final static void createModeFromXml(WindowManager windowManager, String xml) {

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Feb 21, 2019

Contributor

Strange parameters. I'd expect the method to call WindowManager.getDefault() itself.

Shouldn't the method return Mode or null? How can one distinguish between success and failure?

This comment has been minimized.

Copy link
@phipma

phipma Mar 15, 2019

Author Contributor

Yes - Changing to better indicate success or failure.

* @param mode the {@link Mode} to remove
* @return success if the Mode can no longer be found in the Window Manager
*/
public final static boolean removeMode(WindowManager windowManager, Mode mode) {

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Feb 21, 2019

Contributor

What about Mode.Xml.remove()?

This comment has been minimized.

Copy link
@phipma

phipma Mar 18, 2019

Author Contributor

That's an interface - not sure what you mean...

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Mar 18, 2019

Contributor

Yeah, it is an interface, but a newly defined one. E.g. it is possible to put new methods into it, right?

This comment has been minimized.

Copy link
@phipma

phipma Mar 18, 2019

Author Contributor

Sure, but at the point of removing a Mode from the WindowManager, there is no XML in play.
A Mode doesn't have to be an instance of Mode.XML to be removed. As far as I can see all Modes are ModeImpls anyway, which are defined with XML and the Mode.XML interface and ModeUtilities class provides the non-breaking-api way of accessing that.

Now I implemented the remove(), updateConstraints() and createMode() methods as abstract methods of WindowManager as you suggested.

Maybe you can judge these changes on my next push?

* @param windowManager typically WindowManager.getDefault()
* @param xml the XML that was originally produced by {@link #toXml}
*/
public final static void updateModeConstraintsFromXml(WindowManager windowManager, String xml) {

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Feb 21, 2019

Contributor

What about Mode.Xml.updateConstraints(String)?

This comment has been minimized.

Copy link
@phipma

phipma Mar 18, 2019

Author Contributor

Likewise your previous comment.

* Modes directly.
*
* @see ModeXml
*/

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Feb 21, 2019

Contributor

Missing @since tag.

This comment has been minimized.

Copy link
@phipma

phipma Mar 18, 2019

Author Contributor

Hi Jaroslav,

What should I change the version to? Currently manifest contents are:

Manifest-Version: 1.0
OpenIDE-Module: org.openide.windows
OpenIDE-Module-Specification-Version: 6.80
OpenIDE-Module-Localizing-Bundle: org/openide/windows/Bundle.properties
AutoUpdate-Essential-Module: true

This comment has been minimized.

Copy link
@phipma

phipma Mar 18, 2019

Author Contributor

Changing it to 6.81 Hope that's correct.

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach May 15, 2019

Contributor

Not anymore. The current version is 6.81. You need to add something - at least 6.82.

This comment has been minimized.

Copy link
@phipma

phipma May 15, 2019

Author Contributor

OK - changed to 6.82 just now.

@phipma

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Thanks @JaroslavTulach I will make the changes you suggest as soon as I have time.

@lkishalmi

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2019

Just added WIP label until the requested changes arrive.

[NETBEANS-1979] Modifications suggested by Jaroslav on 21 Feb 2019:
Better indication of success or failure of Mode operations,
rename ModeUtility to ModeUtilities,
use abstract methods in WindowManager,
update manifest.mf with 6.81,
update apichanges.xml,
add @SInCE tags.
@phipma

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Changes pushed, please check.
Thanks, Mark

@phipma

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Hi @JaroslavTulach , @lkishalmi, I can't change the work-in-progress label myself - don't know what to do to prompt a review of these latest changes, but they are ready when you are...
Cheers
Mark

@phipma

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Hi Fellas, @geertjanw @JaroslavTulach @lkishalmi do you have scope to progress this PR at all? Should I be doing something...?

@geertjanw

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Is there anything from your point of view that needs to be done? How confident are you of this one?

@geertjanw

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Removed that label, if it helps.

@phipma

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

All done as far as I am concerned, I was hoping Jaroslav would review again and confirm "OpenIDE-Module-Specification-Version: 6.80"
#1135 (comment)

@phipma

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Sorry, I mean "OpenIDE-Module-Specification-Version: 6.81"

phipma added some commits May 15, 2019

@geertjanw

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Hi Mark, awesome work, and what do you think re iCLA, i.e., see this discussion:

https://lists.apache.org/thread.html/b379e8de4d8a7c835a72390a19cbde15bc598038e6bffa878872ed68@%3Cdev.netbeans.apache.org%3E

@junichi11

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Maybe, we should squash and merge.

@geertjanw

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Ideally, @phipma would send in an iCLA first, just need to hear from him how he feels about that.

@junichi11

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Yes :) I don't think that we should merge this PR as it is. So, I think @phipma should rebase and squash himself then push(force) it if possible ;)

@phipma

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

I have sent the ICLA. My git skills are limited - I did a squash before, but not rebase... Can someone outline the steps I should take please?

@junichi11

This comment has been minimized.

Copy link
Member

commented May 17, 2019

I have sent the ICLA.

Great!

@phipma If you accept that we merge your changes as ONE commit, we can do that via the "Squash and merge" button. Otherwise, you should squash your changes properly yourself. (e.g. see https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History)
@matthiasblaesing Could you please give @phipma some advice if possible?
Thanks.

@phipma

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

Please go ahead and use the "squash and merge" button! Let's just get this done!

@junichi11

This comment has been minimized.

Copy link
Member

commented May 17, 2019

@phipma OK, thank you! BTW, did you look at this? #1135 (comment)
If changes are needed, please push commits :)

@geertjanw Let's squash and merge if there is no problem after the iCLA is accepted :)

@phipma

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

Hi @geertjanw, @junichi11 my ICLA was accepted last week, so is there anything else I need to do before you can squash and merge? Let me know if there is, thanks.

@@ -1531,7 +1595,7 @@ public void run() {
if (IDESettings.isGui()) {
// running in GUI mode, but
if (!WindowManagerImpl.getInstance().isVisible()) {
// window manager isn't yet visible => exit for now
// window manager isn't yet visible => exit for now

This comment has been minimized.

Copy link
@junichi11

junichi11 May 24, 2019

Member

Please remove whitespaces.

* @return All the known modes in the XML configuration that they would
* be saved to disk as.
* @throws IOException
*/

This comment has been minimized.

Copy link
@junichi11

junichi11 May 24, 2019

Member

Maybe, the below is better. (* positions)

/** Extract all ModeConfigs in XML.
 *
 * @param wmc
 * @return All the known modes in the XML configuration that they would
 * be saved to disk as.
 * @throws IOException
 */
}

private void writeGroups (WindowManagerConfig wmc) throws IOException {

This comment has been minimized.

Copy link
@junichi11

junichi11 May 24, 2019

Member

Please remove whitespaces.

@junichi11

This comment has been minimized.

Copy link
Member

commented May 24, 2019

@phipma I'm not sure about this area. So I've just had a look at formatting. Please have a look at the comments.

I'm going to merge this later because @JaroslavTulach has already approved this PR.
Any objections?

@junichi11

This comment has been minimized.

Copy link
Member

commented May 24, 2019

I've confirmed that "Mark Phipps" exists in http://people.apache.org/unlistedclas.html.

@junichi11

This comment has been minimized.

Copy link
Member

commented May 24, 2019

@phipma BTW, we should fix the commit message when we squash & merge via the button. Your commit messages are used as a list by default.

e.g.

* Merge remote-tracking branch 'apache/master'

* Merge pull request #1 from apache/master

* Allow any kind of Mode to be created programatically from a ModeConfig

* ...

* ...

Commit message title is this PR one, maybe.

So, please let us know the commit message you expect. Thanks!

@phipma

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

@junichi11 I have made the changes you suggested: removed white space and reformatted comments.
I think the commit message that best describes the original intent of this pull request is, "Allow any kind of Mode to be created programatically from a ModeConfig XML String so that a TopComponent may then be programatically docked into that Mode."

So please use that when you squash and merge.
Many thanks,
Mark

@junichi11 junichi11 merged commit 3b6c4f5 into apache:master May 25, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@junichi11

This comment has been minimized.

Copy link
Member

commented May 25, 2019

@phipma Merged. Thank you for your contribution!

eirikbakke pushed a commit to eirikbakke/incubator-netbeans that referenced this pull request Jun 1, 2019

[NETBEANS-1979] Create Mode from client code (apache#1135)
Allow any kind of Mode to be created programatically from a ModeConfig XML String so that a TopComponent may then be programatically docked into that Mode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.