Merge lp:~tblue/quam-plures/plugins_multiple_instances into lp:quam-plures

[Tilman Blumenbach]

This is an alternate version of lp:~edb/quam-plures/plugins_multiple_instances (see https://code.launchpad.net/~edb/quam-plures/plugins_multiple_instances/+merge/73297 as well). It changes EdB's algorithm slightly:

- We now use the plugin's original code as a base and if it doesn't have one, we use its classname.
- We then look through all other instances of the plugin to find the instance with the highest sequence number. Why do we do this? Imagine you have two instances of a plugin with the code "foo". One instance will have code "foo" while the other one has "foo_2". If you delete instance "foo" and then install a new one, you _could_ end up with the code "foo_2" -- which already exists. My algorithm would assign "foo_3" which is an unique code.
- If we haven't found other instances with sequential codes, we just use $number_of_instances + 1 (two instances already installed => we use 3 => code is "foo_3").

The plugin's name is changed as well (we append " #$seq_number"), just as it being done in EdB's branch.

[EdB (edb)]

Downloading now. I got way too tired yesterday, but some of your feedback on the other version sounded like you hit on things that (a) I was not super-happy with the code and (b) didn't test well. Like deleting out #2 of 3 or the original for example.

Anyway downloading now and it always takes a while. I'll get code review proper happening as I bounce in and out working on the stupid truck.

[EdB (edb)]

Testing now, so far so good, but reading my comment above made me delete the original. I had already deleted #2 of 4 instances and installed #5 successfully. Now I deleted the original. In other words I have "MyPlugin #3" through #5 - 3 cases of it with those numbers as names.

I clicked on "Install new" and it offers me "MyPlugin #4" which while technically is accurate because it would be the fourth it still seems wrong because a plugin with that name already exists. ACTUALLY INSTALLING IT gives me "MyPlugin #6 which is what we would expect. Is this something you think you can improve on?

That is not a show-stopper by the way - just an observation. I am leaving this as a comment only because I have not actually looked at the code. All I've done is take it out for a test run. So far I'm on the way to marking this approved, but would be even happier if the tiny little issue described can be somehow improved.

[Tilman Blumenbach (tblue)]

Ah, you mean you get "Install #4" and the name after installing is "MyPlugin #6"? Hmm. What about making it "Install #6"? Not sure if that would be less confusing...? Maybe even more? Tricky.

[EdB (edb)]

Nice code :)

Approving and perfectly willing to merge in, but would like to hear your thoughts on the minor "it says #4 but #4 exists" issue first.

review: Approve

[EdB (edb)]

Yes that is what I meant. On the install page it is actually saying "Install #4" which really means "the fourth active installation". The confusion comes in with "but I already have #4 as seen on the previous page".

NOT a serious issue. If you think there is a way through it without great effort, or if you think the way it is will be fine then no problem. It's just that I thought "hey that ain't right" although to be honest I was in a code-review state of mind and therefore looking for issues.

[Tilman Blumenbach (tblue)]

Implemented, but it could confuse some users.

review: Needs Resubmitting

[EdB (edb)]

Still awesome, still gonna merge it. I'm figuring Sunday for the 3 ... 4 that aren't from me.

The "this is confusing" can go either way I guess. Hopefully this way is less confusing now that you coded your way to it.

Anyway Sunday looks like a busy day merging for me :)

review: Approve

[Tilman Blumenbach (tblue)]

Going to change this to WIP, I have found something I could easily optimize. :) Won't change the behaviour but allow for slightly nicer code.

[Tilman Blumenbach (tblue)]

Changes made, sorry for the interruption. Code "feels" better now, though. ;)

=== modified file 'qp_inc/plugins/model/_plugins.class.php'
--- qp_inc/plugins/model/_plugins.class.php	2010-12-31 12:12:03 +0000
+++ qp_inc/plugins/model/_plugins.class.php	2011-09-03 13:10:25 +0000
@@ -290,6 +290,9 @@
 	 *
 	 * This handles the indexes, dynamically unregisters a Plugin that does not exist (anymore)
 	 * and instantiates the Plugin's (User)Settings.
+	 * 
+	 * Attention: The code of the plugin is set to NULL if it is found to be not unique!
+	 *            To get the original code, use the $orig_code parameter.
 	 *
 	 * @access protected
 	 * @param string name of plugin class to instantiate and register
@@ -299,9 +302,13 @@
 	 * @param string Path of the .php class file of the plugin.
 	 * @param boolean Must the plugin exist (classfile_path and classname)?
 	 *                This is used internally to be able to unregister a non-existing plugin.
+	 * @param NULL|string If a reference to a variable is given here, it will receive
+	 *                    the code of the plugin we are registering, even if the code
+	 *                    is found to be not unique.
 	 * @return Plugin Plugin ref to newly created plugin; string in case of error
 	 */
-	function & register( $classname, $ID = 0, $priority = -1, $apply_rendering = NULL, $classfile_path = NULL, $must_exists = true )
+	function & register( $classname, $ID = 0, $priority = -1, $apply_rendering = NULL,
+	                     $classfile_path = NULL, $must_exists = true, & $orig_code = NULL )
 	{
 		global $Debuglog, $Messages, $Timer;
 
@@ -438,7 +445,7 @@
 			$Plugin->name = $Plugin->classname;
 		}
 
-		// Memorizes Plugin in code hash array:
+		$orig_code = $Plugin->code;
 		if( ! empty($this->index_code_ID[ $Plugin->code ]) && $this->index_code_ID[ $Plugin->code ] != $Plugin->ID )
 		{ // The plugin's default code is already in use!
 			$Plugin->code = NULL;
 
=== modified file 'qp_inc/plugins/model/_plugins_admin.class.php'
--- qp_inc/plugins/model/_plugins_admin.class.php	2011-02-04 04:06:29 +0000
+++ qp_inc/plugins/model/_plugins_admin.class.php	2011-09-03 13:10:25 +0000
@@ -286,25 +286,43 @@
 
 
 	/**
-	 * Count # of registrations of same plugin.
+	 * Count # of registrations of same plugin. Optionally returns the sequence
+	 * number to be used for the next instance of a plugin as well.
 	 *
 	 * Plugins with negative ID (auto-generated; not installed (yet)) will not get considered.
 	 *
 	 * @param string class name
-	 * @return int # of regs
+	 * @param boolean Return next sequence number as well (will be 1 if there are no other
+	 *                instances of this plugin).
+	 * @return int|array # of regs or array( $regs, $next_seq_number ) if $get_seq_number is true
 	 */
-	function count_regs( $classname )
+	function count_regs( $classname, $get_seq_number = false )
 	{
 		$count = 0;
+		$highest_seq_number = 0;
 
 		foreach( $this->sorted_IDs as $plugin_ID )
 		{
-			$Plugin = & $this->get_by_ID( $plugin_ID );
+			$Plugin = $this->get_by_ID( $plugin_ID );
 			if( $Plugin && $Plugin->classname == $classname && $Plugin->ID > 0 )
 			{
 				$count++;
+				
+				// We assume that nobody has tinkered with the plugin codes in the DB here!
+				if( $get_seq_number && $Plugin->code !== NULL &&
+					preg_match( '/_(\d+)$/', $Plugin->code, $m ) &&
+					$m[1] > $highest_seq_number )
+				{
+					$highest_seq_number = $m[1];
+				}
 			}
 		}
+		
+		if( $get_seq_number )
+		{
+			return array( $count, ( $highest_seq_number ? $highest_seq_number : $count ) + 1 );
+		}
+		
 		return $count;
 	}
 
@@ -491,15 +509,18 @@
 		$this->load_plugins_table();
 
 		// Register the plugin:
-		$Plugin = & $this->register( $classname, 0, -1, NULL, $classfile_path ); // Auto-generates negative ID; New ID will be set a few lines below
+		// Auto-generates negative ID; New ID will be set a few lines below
+		$Plugin = $this->register( $classname, 0, -1, NULL, $classfile_path, 
+						true, $orig_code );
 
 		if( is_string($Plugin) )
 		{ // return error message from register()
 			return $Plugin;
 		}
 
+		list( $regs, $seq_number ) = $this->count_regs( $Plugin->classname, true );
 		if( isset($Plugin->number_of_installs)
-		    && ( $this->count_regs( $Plugin->classname ) >= $Plugin->number_of_installs ) )
+		    && ( $regs >= $Plugin->number_of_installs ) )
 		{
 			$this->unregister( $Plugin, true );
 			$r = T_('The plugin cannot be installed again.');
@@ -540,14 +561,28 @@
 			$Plugin->code = NULL;
 		}
 
+		// Do we have multiple instances of this plugin installed? If yes, we
+		// want a sequential name and code to make the admin's life a bit easier.
+		if( $seq_number > 1 )
+		{
+			if( $Plugin->code === NULL )
+			{	// We need a code.
+				$Plugin->code = ! empty( $orig_code ) ? $orig_code : $classname;
+			}
+			
+			$instance_name = $Plugin->name.' #'.$seq_number;
+			$Plugin->name  = $instance_name;
+			$Plugin->code .= '_'.$seq_number;
+		}
+
 		$Plugin->status = $plug_status;
 
 		// Record into DB
 		$DB->begin();
 
 		$DB->query( '
-				INSERT INTO T_plugins( plug_classname, plug_priority, plug_code, plug_apply_rendering, plug_version, plug_status )
-				VALUES( "'.$classname.'", '.$Plugin->priority.', '.$DB->quote($Plugin->code).', '.$DB->quote($Plugin->apply_rendering).', '.$DB->quote($Plugin->version).', '.$DB->quote($Plugin->status).' ) ' );
+				INSERT INTO T_plugins( plug_classname, plug_priority, plug_code, plug_apply_rendering, plug_version, plug_name, plug_status )
+				VALUES( "'.$classname.'", '.$Plugin->priority.', '.$DB->quote($Plugin->code).', '.$DB->quote($Plugin->apply_rendering).', '.$DB->quote($Plugin->version).', '.$DB->quote($Plugin->name).', '.$DB->quote($Plugin->status).' ) ' );
 
 		// Unset auto-generated ID info
 		unset( $this->index_ID_Plugins[ $Plugin->ID ] );
@@ -562,8 +597,9 @@
 				'plug_classname' => $Plugin->classname,
 				'plug_code' => $Plugin->code,
 				'plug_apply_rendering' => $Plugin->apply_rendering,
+				'plug_version' => $Plugin->version,
+				'plug_name' => $Plugin->name,
 				'plug_status' => $Plugin->status,
-				'plug_version' => $Plugin->version,
 			);
 		$this->sorted_IDs[$key] = $Plugin->ID;
 
@@ -589,6 +625,10 @@
 			$Debuglog->add( 'Unregistered plugin, because PluginInit returned false.', 'plugins' );
 			$Plugin = '';
 		}
+		else if( isset( $instance_name ) )
+		{	// $Plugin->PluginInit( $tmp_params ) lost our new name :(
+			$Plugin->name = $instance_name;
+		}
 
 		if( ! defined('QP_IS_INSTALLING') || ! QP_IS_INSTALLING )
 		{ // do not sort, if we're installing/upgrading.. instantiating Plugins might cause a fatal error!
 
=== modified file 'qp_inc/plugins/plugins.ctrl.php'
--- qp_inc/plugins/plugins.ctrl.php	2010-12-31 12:12:03 +0000
+++ qp_inc/plugins/plugins.ctrl.php	2011-09-03 13:10:25 +0000
@@ -1021,8 +1021,6 @@
 
 		if( $edit_Plugin->ID < 1 )
 		{ // add "Install NOW" submit button (if not already installed)
-			$registrations = $admin_Plugins->count_regs($edit_Plugin->classname);
-
 			if( ! isset( $edit_Plugin->number_of_installs )
 					|| ( $admin_Plugins->count_regs($edit_Plugin->classname) < $edit_Plugin->number_of_installs ) )
 			{ // number of installations are not limited or not reached yet
 
=== modified file 'qp_inc/plugins/views/_plugin_list_available.view.php'
--- qp_inc/plugins/views/_plugin_list_available.view.php	2010-12-31 12:12:03 +0000
+++ qp_inc/plugins/views/_plugin_list_available.view.php	2011-09-03 13:10:25 +0000
@@ -166,7 +166,7 @@
 		$Table->display_col_end();
 
 		$Table->display_col_start();
-			$registrations = $admin_Plugins->count_regs($loop_Plugin->classname);
+			list( $registrations, $seq_number ) = $admin_Plugins->count_regs( $loop_Plugin->classname, true );
 
 			if( $current_User->check_perm( 'options', 'edit', false )
 					&& ( ! isset( $loop_Plugin->number_of_installs )
@@ -175,9 +175,9 @@
 				?>
 				[<a href="<?php echo $dispatcher ?>?ctrl=plugins&amp;action=install&amp;plugin=<?php echo rawurlencode($loop_Plugin->classname) ?>"><?php
 					echo T_('Install');
-					if( $registrations )
+					if( $seq_number > 1 )
 					{	// This plugin is already installed
-						echo ' #'.($registrations+1);
+						echo ' #', $seq_number;
 					}
 					?></a>]
 				<?php