Singleton JFrame not appearing on script start

[Tom]

Not sure why, I've set it to visible and all that jazz. 98% sure I'm just missing something.

[fixthissite]

invokeLater posts your code to the Event Queue to be executed on the Event Dispatch Thread. When it will be executed is unknown. To ensure the calling thread waits until the Event Dispatch Thread has finished executing your Swing code, use invokeAndWait. Although, I can't promise this will fix it, seeing how you only show a small portion of your code..

 

Also, I heard OSBot now has support for Java 8. I suggest using a lambda to pass in the run() method

Edited April 24, 2015 by fixthissite

[Botre]

invokeLater posts your code to the Event Queue to be executed on the Event Dispatch Thread. When it will be executed is unknown. To ensure the calling thread waits until the Event Dispatch Thread has finished executing your Swing code, use invokeAndWait. Although, I can't promise this will fix it, seeing how you only show a small portion of your code

 

^

 

(might want to show us how you're instantiating the singleton)

Edited April 24, 2015 by Botre

[Tom]

private GUI() {
		instance = this;
		isStarted = false;
		karamja = false;
		barehanded = false;
		initComponents();
		loc = (Locations) cmbLocation.getSelectedItem();
	}
	
	public static GUI getInstance(){
		return instance;
	}

Changing it to invokeAndWait didn't seem to help.

 

I've instantiated it like

private GUI gui;

 

invoke stuff

gui = GUI.getInstance();

 

Also, how would I go about using lamdas with this runnable, ive done it with other things but this just dun wanna work

 

Thanks

Edited April 24, 2015 by Mykindos

[fixthissite]

private GUI() {
		instance = this;
		isStarted = false;
		karamja = false;
		barehanded = false;
		initComponents();
		loc = (Locations) cmbLocation.getSelectedItem();
	}
	
	public static GUI getInstance(){
		return instance;
	}

And is instantiated like

private GUI gui;

 

invoke stuff

gui = GUI.getInstance();

 

Also, how would I go about using lamdas with this runnable, ive done it with other things but this just dun wanna work

 

Thanks

 

Please show your initComponents() method; I was hoping to see your actual Swing code, make sure everything is fine. Also, Im no fan of singletons. But if you're gonna have one, use an enum for it.

 

As for the lambdas, they work for functional interfaces (interfaces with only 1 method). Instead of creating an anonymous class, specify an identifier for each parameter of the method (or use () if there is no parameters), then point to a statement block using ->:

EventQueue.invokeAndWait(() -> {

});

If your lambda only has 1 statement, you can omit the { }. If you had an enum, you would only need 1 statement:

enum FrameSingleton {
     FRAME;

     //define your frame's behavior and state
}

It'll instantiate as soon as you refer to the enum. You would be able to do

EventQueue.invokeAndWait(() -> FRAME.setVisible(true));

Edited April 24, 2015 by fixthissite

[Tom]

Please show your initComponents() method; I was hoping to see your actual Swing code, make sure everything is fine. Also, Im no fan of singletons. But if you're gonna have one, use an enum for it.

 

As for the lambdas, they work for functional interfaces (interfaces with only 1 method). Instead of creating an anonymous class, specify an identifier for each parameter of the method (or use () if there is no parameters), then point to a statement block using ->:

 

EventQueue.invokeAndWait(() -> {

 

});

 

If your lambda only has 1 statement, you can omit the { }

	private void initComponents() {
		buttonStart = new JButton();
		lblType = new JLabel();
		cmbType = new JComboBox<String>();
		lblLocation = new JLabel();
		cmbLocation = new JComboBox<Locations>(Locations.values());
		lblFish = new JLabel();
		cmbFish = new JComboBox<String>();
		chkHand = new JCheckBox();

		cmbType.addItem("Bank");
		cmbType.addItem("Drop");
		loc = (Locations) cmbLocation.getSelectedItem();
		for(String str : loc.getFishes()){
			cmbFish.addItem(str);
		}

		setTitle("osFisher by Mykindos");
		Container contentPane = getContentPane();
		contentPane.setLayout(null);


		buttonStart.setText("Start Script");
		buttonStart.setFont(new Font("Tahoma", Font.BOLD, 18));
		buttonStart.addActionListener(new ActionListener() {
			@Override
			public void actionPerformed(ActionEvent e) {
				buttonStartActionPerformed(e);
			}
		});
		contentPane.add(buttonStart);
		buttonStart.setBounds(85, 230, 145, 50);

		chkHand.setText("Barehanded fishing");
		lblType.setText("Type of Fishing:");
		lblType.setFont(new Font("Tahoma", Font.PLAIN, 18));
		contentPane.add(lblType);
		lblType.setBounds(10, 20, 140, 30);
		contentPane.add(cmbType);
		cmbType.setBounds(140, 15, 170, 40);
		contentPane.add(chkHand);
		chkHand.setBounds(140, 170, 170, 40);

		lblLocation.setText("Location:");
		lblLocation.setFont(new Font("Tahoma", Font.PLAIN, 18));
		contentPane.add(lblLocation);
		lblLocation.setBounds(65, 65, 75, 50);


		cmbLocation.addActionListener(new ActionListener() {
			@Override
			public void actionPerformed(ActionEvent e) {
				cmbLocationActionPerformed(e);
				cmbLocationActionPerformed(e);
			}
		});
		contentPane.add(cmbLocation);
		cmbLocation.setBounds(140, 70, 170, 40);


		lblFish.setText("Fish:");
		lblFish.setFont(new Font("Tahoma", Font.PLAIN, 18));
		contentPane.add(lblFish);
		lblFish.setBounds(100, 125, 40, 45);
		contentPane.add(cmbFish);
		cmbFish.setBounds(140, 125, 170, 40);

		{ 
			Dimension preferredSize = new Dimension();
			for(int i = 0; i < contentPane.getComponentCount(); i++) {
				Rectangle bounds = contentPane.getComponent(i).getBounds();
				preferredSize.width = Math.max(bounds.x + bounds.width, preferredSize.width);
				preferredSize.height = Math.max(bounds.y + bounds.height, preferredSize.height);
			}
			Insets insets = contentPane.getInsets();
			preferredSize.width += insets.right;
			preferredSize.height += insets.bottom;
			contentPane.setMinimumSize(preferredSize);
			contentPane.setPreferredSize(preferredSize);
		}
		pack();
		setLocationRelativeTo(getOwner());

	}

Quite a bit there, has always worked beforehand though.

Wouldn't be surprised if I accidentally deleted something since I rewrote a lot of my script today.

[fixthissite]

So your Swing code looks fine. But after taking a second look at your singleton design, you never lazily initialize your singleton in the getInstance() method. This makes me assume you already have an instance created:

class Single {
     private static Single SINGLE = new Single();

     public Single() {
          instance = this;
     }

     public static Single getInstance() {
          return SINGLE;
     }  
}

The problem with this design is that you instantiate the object in the field variable, then replace that object immediately after.

 

Check this out:

public Single() {
     System.out.println(this == SINGLE); //prints false
     SINGLE = this;
}

This could have been avoided by ensuring the field was final. Try removing instance = this

Edited April 24, 2015 by fixthissite

[Tom]

Edit: Ok I figured it out, thanks @Fixmysite

 

 

And yeah because it was a singleton I was never actually initializing it properly.

 

Just having private static GUI instance = new GUI(); worked fine

Edited April 24, 2015 by Mykindos

[fixthissite]

I replaced instance = this with instance = new GUI();

Even still it didn't work.

 

So this is what I have changed so far.

 

GUI:

Also tried removing instance = new GUI(), but as I expected it didnt make a difference

It would be extremely helpful if I saw the entire singleton class. Im going based off assumpsions right now cause you're leaving out important info, such as if the field is already initialized, or if you're replacing the singleton instance by accident (make the field final). You should NOT be replacing the object in the field AT ANY TIME. 

 

You need to call `getInstance()` in your Runnable as well. Not sure why you took that out; you meed to make sure ALL swing code executes on the EDT. That littl3 change could be contributing to the problem..

[Tom]

It would be extremely helpful if I saw the entire singleton class. Im going based off assumpsions right now cause you're leaving out important info, such as if the field is already initialized, or if you're replacing the singleton instance by accident (make the field final). You should NOT be replacing the object in the field AT ANY TIME. 

 

You need to call `getInstance()` in your Runnable as well. Not sure why you took that out; you meed to make sure ALL swing code executes on the EDT. That littl3 change could be contributing to the problem..

 

Edited the above post ^

 

Thanks!

[fixthissite]

Edit: Ok I figured it out, thanks @Fixmysite

 

 

And yeah because it was a singleton I was never actually initializing it properly.

 

Just having private static GUI instance = new GUI(); worked fine

This is why I always recommend using enum for singletons. Its almost impossible to mess up a singleton using an enum. Glad to hear you got it working

Edited April 24, 2015 by fixthissite

[Joseph]

That brain fart of your op lol..

Change your the gui # getInstance make it return new Gui():

Using a static method to return a static variable doesn't cut it. Since your private field was never initialized.

(

Edited April 24, 2015 by josedpay

[fixthissite]

That brain fart of your op lol..

Change your the gui # getInstance make it return new Gui():

Using a static method to return a static variable doesn't cut it. Since your private field was never initialized.

(

Making it return new GUI() doesn't ensure only a single instance of that class will exist within memory. One of the most important aspects is ensuring new GUI is only called once. Lazy initialization would make that suggestion more reasonable:

public static GUI getInstance() {
     if(instance == null)
          instance = new GUI();

     return instance;
}

But honestly, creating singletons using classes is extremely tedious, as well as dangerous. For example, 2 threads call getInstance(). First thread sees that instance null, initializes it. Right after thread 1 sees that instance is null, but before it has a chance to initialize it, thread 2 could also see that it's null and attempt to initialize. To prevent this, you'd need double check locking. But even that doesn't prevent new instances through reflection or deserialialization. To me, it doesn't make sense to use a class for a singleton anymore; too much tedious work. Just use an enum. Might actually write a guide on this

Edited April 24, 2015 by fixthissite

[Botre]

Making it return new GUI() doesn't ensure only a single instance of that class will exist within memory. One of the most important aspects is ensuring new GUI is only called once. Lazy initialization would make that suggestion more reasonable:

public static GUI getInstance() {
     if(instance == null)
          instance = new GUI();

     return instance;
}

But honestly, creating singletons using classes is extremely tedious, as well as dangerous. For example, 2 threads call getInstance(). First thread sees that instance null, initializes it. Right after thread 1 sees that instance is null, but before it has a chance to initialize it, thread 2 could also see that it's null and attempt to initialize. To prevent this, you'd need double check locking. But even that doesn't prevent new instances through reflection or deserialialization. To me, it doesn't make sense to use a class for a singleton anymore; too much tedious work. Just use an enum. Might actually write a guide on this

package org.bjornkrols.design_patterns.lazy_singleton;

/**
 * @author 		Bjorn Krols (Botre)
 * @version		0.0
 * @since		March 6, 2015
 */

public final class LazySingleton {

	// Instance is not created until getInstance() is called.
	private static volatile LazySingleton instance = null;

	private LazySingleton() {
		// Do not delete or make accessible.
		// Exists only to prevent creating multiple instances.
	}

	public static LazySingleton getInstance() {
		if (instance == null) {
			synchronized (LazySingleton.class) {
				// Double check, synchronized block.
				if (instance == null) {
					instance = new LazySingleton();
				}
			}
		}
		return instance;
	}

}

[fixthissite]

package org.bjornkrols.design_patterns.lazy_singleton;

/**
 * @author 		Bjorn Krols (Botre)
 * @version		0.0
 * @since		March 6, 2015
 */

public final class LazySingleton {

	// Instance is not created until getInstance() is called.
	private static volatile LazySingleton instance = null;

	private LazySingleton() {
		// Do not delete or make accessible.
		// Exists only to prevent creating multiple instances.
	}

	public static LazySingleton getInstance() {
		if (instance == null) {
			synchronized (LazySingleton.class) {
				// Double check, synchronized block.
				if (instance == null) {
					instance = new LazySingleton();
				}
			}
		}
		return instance;
	}

}

 

Prevents inconsistencies. But can be a victim to deserialization (you can serialize without needing to implement serializable by declaring readObject and writeObject methods) and instantiation through reflection. Not to mention,

public enum Singleton {
     INSTANCE;
}

Is a lot let verbose ;)