--===============7863754246837541919== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119115/#review62752 ----------------------------------------------------------- Thanks for the update! Sorry for the late response - I was a bit busy at work last week. Looks good already (and much better than the last version, thanks for fixing the issues!). I still have a few questions though. dolphin/src/dolphinmainwindow.cpp Maybe the active view could be a parameter of the signal, i.e., activeViewChanged(DolphinViewContainer* newViewContainer)? This access would not be needed then. And maybe the activeViewContainer() function in DolphinTabWidget could even be removed? dolphin/src/dolphinmainwindow.cpp The old code simply kept the connections to the signals of all views and URL navigators at all times. Could you comment on why you propose to introduce the disconnections and connections now? dolphin/src/dolphintabwidget.cpp As far as I can see, this signal does not exist at all? dolphin/src/dolphintabwidget.cpp It seems that this signal does not exist either. - Frank Reininghaus On July 15, 2014, 9:26 p.m., Emmanuel Pescosta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119115/ > ----------------------------------------------------------- > > (Updated July 15, 2014, 9:26 p.m.) > > > Review request for Dolphin. > > > Repository: kde-baseapps > > > Description > ------- > > Implemented DolphinTabWidget class to encapsulate the tab handling from DolphinMainWindow. > > Removed the method for text squeezing because the QTabBar can do this automatically. > > The tab sizes are different, any idea how to fix this? > > > Diffs > ----- > > dolphin/src/CMakeLists.txt 7b0210a > dolphin/src/dolphinmainwindow.h 9c7f185 > dolphin/src/dolphinmainwindow.cpp 41bd626 > dolphin/src/dolphintabwidget.h PRE-CREATION > dolphin/src/dolphintabwidget.cpp PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/119115/diff/ > > > Testing > ------- > > Works great so far. > > > Thanks, > > Emmanuel Pescosta > > --===============7863754246837541919== MIME-Version: 1.0 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 7bit
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119115/ 

Thanks for the update! Sorry for the late response - I was a bit busy at work last week.


Looks good already (and much better than the last version, thanks for fixing the issues!). I still have a few questions though.

dolphin/src/dolphinmainwindow.cpp (Diff revision 3) 
void DolphinMainWindow::slotPlaceActivated(const KUrl& url)
1219 
    setActiveViewContainer(tabPage->activeViewContainer());
 920 
    DolphinViewContainer* newViewContainer = m_tabWidget->activeViewContainer();
 
Maybe the active view could be a parameter of the signal, i.e.,


activeViewChanged(DolphinViewContainer* newViewContainer)?


This access would not be needed then. And maybe the activeViewContainer() function in DolphinTabWidget could even be removed?

dolphin/src/dolphinmainwindow.cpp (Diff revision 3) 
void DolphinMainWindow::setActiveViewContainer(DolphinViewContainer* viewContainer)
 


  

 



 
 

 

  

    1224
    
    Q_ASSERT(viewContainer);

    925
    
    if (oldViewContainer) {

  


 




 


  
The old code simply kept the connections to the signals of all views and URL navigators at all times.


Could you comment on why you propose to introduce the disconnections and connections now?

 













 
  

   
    dolphin/src/dolphintabwidget.cpp
    

     (Diff revision 3)

    
   
  

 



 
 

 

  

    
    

    37
    
    connect(this, SIGNAL(tabCloseRequested(int)),

  


 




 


  
As far as I can see, this signal does not exist at all?

 













 
  

   
    dolphin/src/dolphintabwidget.cpp
    

     (Diff revision 3)

    
   
  

 



 
 

 

  

    
    

    39
    
    connect(this, SIGNAL(currentChanged(int)),

  


 




 


  
It seems that this signal does not exist either.

 









- Frank Reininghaus






On July 15th, 2014, 9:26 p.m. UTC, Emmanuel Pescosta wrote:












 

  


Review request for Dolphin.


By Emmanuel Pescosta.




Updated July 15, 2014, 9:26 p.m.












 Repository: 
kde-baseapps





Description 

 

 

  
   
Implemented DolphinTabWidget class to encapsulate the tab handling from DolphinMainWindow.


Removed the method for text squeezing because the QTabBar can do this automatically.


The tab sizes are different, any idea how to fix this?

  		
 






Testing 



 

  
   
Works great so far.

  		
 






Diffs 


    

 
  • dolphin/src/CMakeLists.txt (7b0210a)
    • 

 
  • dolphin/src/dolphinmainwindow.h (9c7f185)
    • 

 
  • dolphin/src/dolphinmainwindow.cpp (41bd626)
    • 

 
  • dolphin/src/dolphintabwidget.h (PRE-CREATION)
    • 

 
  • dolphin/src/dolphintabwidget.cpp (PRE-CREATION)
    • 





View Diff







  		
 











  

 



--===============7863754246837541919==--