Restore the tab's history when restoring a tab/session

Review Request #100604 - Created Feb. 7, 2011 and submitted

Information
Anton Kreuzkamp
rekonq
master
Reviewers
rekonq
Restore the history of the tab when restoring a tab from the list of closed tabs, by open last closed tab or by restoring the session.
Therefore I ported the session-file to xml, as it allows more complex session-files (which is also needed for multiple sessions support).
(Doesn't require a qtwebkit-patch anymore)
tested and everything seems to work fine.
Anton Kreuzkamp
Andrea Diamantini
Pierre Rossi
Anton Kreuzkamp
Review request changed
Andrea Diamantini
This patch is not working here.
  1. What's the problem? Does it not compile? (error-message?) Does it crash? (backtrace?) Or does it simply not work? (what exactly? open a closed tab/restoring the session/both? what does the session-file look like?(is it xml?))
  2. This patch is working for me with rekonq-git (at least I found no errors).
  3. not working in the sense that no history is saved. At least I don't see it on restore.
  4. Does the session-file exist (should be ~/.kde/share/apps/rekonq/session)? How does it look like?
  5. I tried reinstalling your patch, deleting my session file and now the history save/restore works well. Some problems with previous version? Maybe. 
    
    I don't understand the changes you did to the code. What's the goal of changing the "recentlyClosedTabs" structure? Can it be (re)moved at all in the session manager? And why are you using it to store the tabs information? Moreover, your changes broke the closed tabs contextual menu and the closed tabs page.
  6. >I tried reinstalling your patch, deleting my session file and now the history save/restore works well. Some problems with previous version? Maybe.
    Nice to hear that it's solved now :)
    
    >I don't understand the changes you did to the code.
    Well I can try to explain you, whatever you don't understand. Comments at the exact places in the code would be helpful.
    
    >What's the goal of changing the "recentlyClosedTabs" structure?
    Well it restores the history of closed tabs as well..
    
    >Can it be (re)moved at all in the session manager?
    I don't quite understand what you mean by that. I can only say, I tried to change the structure of the code (where which methods are, etc.) as little as possible.
    
    >And why are you using it to store the tabs information?
    What exactly? If you mean the Buffer with the history, it's the only way to save the history, so that I can restore it later on.
    
    >Moreover, your changes broke the closed tabs contextual menu and the closed tabs page.
    More information would be desirable, I will try to fix every bug, that occurs to me. I can just say, everything I can test at the moment (which is just the context-menu) works here. But I can't remember any difficulties with the closed tabs page.
    But I will test it again with a current checkout, tomorrow ;)
  7. I can read your code and understand what a change does. What I don't see is WHY you did. 
    
    I tried firefox and chrome and I didn't see the feature of remember closed tabs history on restart. In general, I think this is a "not wanted" feature (too much intrusive and different from habits: I sure cannot remember last closed tab from previous session). So, I'm ok with the restoring tab history on reopening last closed tab. I don't like the restore tabs history "session" part.
Andrea Diamantini
Need someone more reviewing this BIG (and very nice) bunch of changes. Pierre?
  1. taking another look... :)
Pierre Rossi
It looks globally sane, at least based on the diff to the old code. I think it's a pretty important feature, so we should probably go for it and see how it goes.
Commit Hook
This review has been submitted with commit a0315947c024a3be1d35b4700af7fa653272093e by Anton Kreuzkamp to branch master.
Loading...