Bug 263021

Summary:	Patch review closes all open documents from other worksets
Product:	[Developer tools] kdevplatform	Reporter:	András Manţia <amantia>
Component:	patchreview	Assignee:	kdevelop-bugs-null
Status:	RESOLVED FIXED
Severity:	normal	CC:	david.nolden.kde, reuben_p
Priority:	NOR
Version:	git master
Target Milestone:	1.2.0
Platform:	Compiled Sources
OS:	Linux
Latest Commit:		Version Fixed In:
Sentry Crash Report:

Description András Manţia 2011-01-13 14:34:44 UTC

Krevplatform & kdevelop master: open a project that uses git or svn, open two files from it, right click on the project root and select Commit. The workset is switched to Review and you see the patch (if any file was modified). Cancel the review or commit, and see that the two open documents are closed. You can also verify this without closing the review, just switch back to Code or Debug working sets...
 I added some debug output to the patchreview.cpp to see what is happening (unfortunately lost it, so I write here what I did)

line 1407: a debug to check what is added to the documents map. This shows that the two originally opened file is added.

line 1415: to check what is removed from the documents map: only the created .patch file is verified here, not the opened documents, so the opened documents remain in the documents map.

line 1452: to check what view are closed. As expected the two originally opened documents are closed. This is the bug.

The problem might be that the workin set is switched in the beginning of the method, before line 1407, still line 1407 returns those documents that are opened in other worksets, not only those that are opened in the review workset.

As I understood the code tries to close open documents in the review workset that are not part of the review (the patch), but instead closes all open documents.

Comment 1 David Nolden 2011-01-13 16:10:09 UTC

The problem is that you are using a working-set (one of those icons) which is associated to the review area. The review area always clears this working set when a new review is started, and it puts in the documents affecting the patch.

You can workaround it by simply closing the at working set in the non-review area. However we should also somehow make this confusion impossible to happen for people who aren't aware of working sets.

Comment 2 David Nolden 2011-01-13 16:45:07 UTC

commit 0bae115b30e3de233abbf212e973e96501d047b5
branch master
Author: David Nolden <david.nolden.kde@art-master.de>
Date:   Thu Jan 13 16:42:51 2011 +0100

    Make sure that working sets are unique to the patch-review area before changing them.
    BUG: 263021

diff --git a/plugins/patchreview/patchreview.cpp b/plugins/patchreview/patchreview.cpp
index 3575636..4925226 100644
--- a/plugins/patchreview/patchreview.cpp
+++ b/plugins/patchreview/patchreview.cpp
@@ -1351,6 +1351,7 @@ void PatchReviewPlugin::cancelReview()
     
     Sublime::MainWindow* w = dynamic_cast<Sublime::MainWindow*>(ICore::self()->uiController()->activeMainWindow());
     if(w->area()->objectName() == "review") {
+      setUniqueWorkingSet(); // Make the working-set unique, so that we don't affect other areas
       w->area()->clearViews();
       ICore::self()->uiController()->switchToArea("code", KDevelop::IUiController::ThisWindow);
     }
@@ -1385,6 +1386,35 @@ void PatchReviewPlugin::startReview(IPatchSource* patch, IPatchReview::ReviewMod
   QMetaObject::invokeMethod(this, "updateReview", Qt::QueuedConnection);
 }
 
+void PatchReviewPlugin::switchAreaAndMakeWorkingSetUique()
+{
+  Sublime::MainWindow* w = dynamic_cast<Sublime::MainWindow*>(ICore::self()->uiController()->activeMainWindow());
+  if (w->area()->objectName() != "review")
+    ICore::self()->uiController()->switchToArea("review", KDevelop::IUiController::ThisWindow);
+
+  setUniqueWorkingSet();
+}
+
+bool PatchReviewPlugin::isWorkingSetUnique() const
+{
+  Sublime::MainWindow* w = dynamic_cast<Sublime::MainWindow*>(ICore::self()->uiController()->activeMainWindow());
+  foreach(Sublime::Area* area, w->areas())
+    if(area != w->area() && area->workingSet() == w->area()->workingSet())
+      return false;
+  return true;
+}
+
+void PatchReviewPlugin::setUniqueWorkingSet()
+{
+  Sublime::MainWindow* w = dynamic_cast<Sublime::MainWindow*>(ICore::self()->uiController()->activeMainWindow());
+  
+  if(!w->area()->workingSet().startsWith("review"))
+    w->area()->setWorkingSet("review");
+  
+  while(!isWorkingSetUnique())
+    w->area()->setWorkingSet(QString("review_%1").arg(rand() % 10000));
+}
+
 void PatchReviewPlugin::updateReview()
 {
   if(!m_patch)
@@ -1393,12 +1423,7 @@ void PatchReviewPlugin::updateReview()
   m_updateKompareTimer->stop();
   updateKompareModel();
 
-  Sublime::MainWindow* w = dynamic_cast<Sublime::MainWindow*>(ICore::self()->uiController()->activeMainWindow());
-  if (w->area()->objectName() != "review")
-    ICore::self()->uiController()->switchToArea("review", KDevelop::IUiController::ThisWindow);
-
-  if(!w->area()->workingSet().startsWith("review"))
-    w->area()->setWorkingSet("review");
+  switchAreaAndMakeWorkingSetUique();
   
   if(!m_modelList.get())
     return;
@@ -1455,6 +1480,7 @@ void PatchReviewPlugin::updateReview()
     }
   }
 
+  Sublime::MainWindow* w = dynamic_cast<Sublime::MainWindow*>(ICore::self()->uiController()->activeMainWindow());
   // Close views for documents that were loaded from the working set, but are not in the patch
   QList<IDocument*> documentsList = documents.values();
   foreach(Sublime::View* view, w->area()->views()) {
diff --git a/plugins/patchreview/patchreview.h b/plugins/patchreview/patchreview.h
index 5815822..c517f34 100644
--- a/plugins/patchreview/patchreview.h
+++ b/plugins/patchreview/patchreview.h
@@ -198,6 +198,14 @@ private Q_SLOTS:
     void exporterSelected(QAction* action);
     
 private:
+    // Switches to the review area, 
+    // makes sure that the working set active in the current area starts with "review" and
+    // is not active in any other area. Creates new working sets if required.
+    void switchAreaAndMakeWorkingSetUique();
+    // Returns whether the current working set is active only in this area
+    bool isWorkingSetUnique() const;
+    // Makes sure that this working set is active only in this area, and that its name starts with "review".
+    void setUniqueWorkingSet();
   
     QList<KDevelop::IPatchSource::Ptr> m_knownPatches;

Comment 3 David Nolden 2011-01-13 21:57:30 UTC

commit 55248ee678e159094b8c87228a34d4e3ae023224
branch 1.2
Author: David Nolden <david.nolden.kde@art-master.de>
Date:   Thu Jan 13 16:42:51 2011 +0100

    backport from master: Make sure that working sets are unique to the patch-review area before changing them.
    CCBUG: 263021

diff --git a/plugins/patchreview/patchreview.cpp b/plugins/patchreview/patchreview.cpp
index 3575636..4925226 100644
--- a/plugins/patchreview/patchreview.cpp
+++ b/plugins/patchreview/patchreview.cpp
@@ -1351,6 +1351,7 @@ void PatchReviewPlugin::cancelReview()
     
     Sublime::MainWindow* w = dynamic_cast<Sublime::MainWindow*>(ICore::self()->uiController()->activeMainWindow());
     if(w->area()->objectName() == "review") {
+      setUniqueWorkingSet(); // Make the working-set unique, so that we don't affect other areas
       w->area()->clearViews();
       ICore::self()->uiController()->switchToArea("code", KDevelop::IUiController::ThisWindow);
     }
@@ -1385,6 +1386,35 @@ void PatchReviewPlugin::startReview(IPatchSource* patch, IPatchReview::ReviewMod
   QMetaObject::invokeMethod(this, "updateReview", Qt::QueuedConnection);
 }
 
+void PatchReviewPlugin::switchAreaAndMakeWorkingSetUique()
+{
+  Sublime::MainWindow* w = dynamic_cast<Sublime::MainWindow*>(ICore::self()->uiController()->activeMainWindow());
+  if (w->area()->objectName() != "review")
+    ICore::self()->uiController()->switchToArea("review", KDevelop::IUiController::ThisWindow);
+
+  setUniqueWorkingSet();
+}
+
+bool PatchReviewPlugin::isWorkingSetUnique() const
+{
+  Sublime::MainWindow* w = dynamic_cast<Sublime::MainWindow*>(ICore::self()->uiController()->activeMainWindow());
+  foreach(Sublime::Area* area, w->areas())
+    if(area != w->area() && area->workingSet() == w->area()->workingSet())
+      return false;
+  return true;
+}
+
+void PatchReviewPlugin::setUniqueWorkingSet()
+{
+  Sublime::MainWindow* w = dynamic_cast<Sublime::MainWindow*>(ICore::self()->uiController()->activeMainWindow());
+  
+  if(!w->area()->workingSet().startsWith("review"))
+    w->area()->setWorkingSet("review");
+  
+  while(!isWorkingSetUnique())
+    w->area()->setWorkingSet(QString("review_%1").arg(rand() % 10000));
+}
+
 void PatchReviewPlugin::updateReview()
 {
   if(!m_patch)
@@ -1393,12 +1423,7 @@ void PatchReviewPlugin::updateReview()
   m_updateKompareTimer->stop();
   updateKompareModel();
 
-  Sublime::MainWindow* w = dynamic_cast<Sublime::MainWindow*>(ICore::self()->uiController()->activeMainWindow());
-  if (w->area()->objectName() != "review")
-    ICore::self()->uiController()->switchToArea("review", KDevelop::IUiController::ThisWindow);
-
-  if(!w->area()->workingSet().startsWith("review"))
-    w->area()->setWorkingSet("review");
+  switchAreaAndMakeWorkingSetUique();
   
   if(!m_modelList.get())
     return;
@@ -1455,6 +1480,7 @@ void PatchReviewPlugin::updateReview()
     }
   }
 
+  Sublime::MainWindow* w = dynamic_cast<Sublime::MainWindow*>(ICore::self()->uiController()->activeMainWindow());
   // Close views for documents that were loaded from the working set, but are not in the patch
   QList<IDocument*> documentsList = documents.values();
   foreach(Sublime::View* view, w->area()->views()) {
diff --git a/plugins/patchreview/patchreview.h b/plugins/patchreview/patchreview.h
index 5815822..c517f34 100644
--- a/plugins/patchreview/patchreview.h
+++ b/plugins/patchreview/patchreview.h
@@ -198,6 +198,14 @@ private Q_SLOTS:
     void exporterSelected(QAction* action);
     
 private:
+    // Switches to the review area, 
+    // makes sure that the working set active in the current area starts with "review" and
+    // is not active in any other area. Creates new working sets if required.
+    void switchAreaAndMakeWorkingSetUique();
+    // Returns whether the current working set is active only in this area
+    bool isWorkingSetUnique() const;
+    // Makes sure that this working set is active only in this area, and that its name starts with "review".
+    void setUniqueWorkingSet();
   
     QList<KDevelop::IPatchSource::Ptr> m_knownPatches;

Comment 4 András Manţia 2011-01-14 09:44:20 UTC

Thanks for the fix. Honestly, I have no idea what you mean with my usage of working sets... I use it just as KDevelop comes by default (at least I don't remember changing anything). When I start to Debug, it switched to the Debug tab at the top. When I start to commit/review, it switches to the Review tab. This was like this for "forever".