Vis4Sense / HistoryMap

http://sensemap.io
60 stars 7 forks source link

Additional history map nodes when reopening closed tab via highlights #101

Closed HumzahJavid closed 6 years ago

HumzahJavid commented 6 years ago

The issue

What is the issue?

Two additional nodes appear on the history map, when a text/note node is clicked for a node/tab which was previously closed. One of the additional nodes is a node which displays the same favicon/icon and webpage title as the original node, when this node is clicked on it opens a blank "new tab" tab. The other additional node only has no icon/favicon, only displaying the url of the original node, when this node is clicked on it opens the webpage which had the original highlights, with all the highlights in place.

To re-create the issue:

Start the extension, make sure the history map is active. Open any webpage and use any of the annotation features (highlighting text or adding a note to highlighted text). Now close the tab and return to the historymap, click on any of the highlight nodes of the tab which was just closed. The tab will reopen, with all the highlights in place. However on the historymap page the 2 additional nodes will also appear as previously described

The additional nodes as they appear in History Map

issue with duplicated nodes

Branch and Commit where this issue can be replicated

This issue can be seen in branch rebuild-mvc-historymap-highlights commit c7be9a7 (check earliest commit with issue)

kaidatavis commented 6 years ago
kaidatavis commented 6 years ago

Related to #92 Turns out not the case.

kaidatavis commented 6 years ago

Humzah to contact Phong to find out how this was done in the SenseMap (master branch).

kaidatavis commented 6 years ago

Phong's comment from his email

On 27 Jun 2018, at 06:14, Nguyen, Phong Phong.Nguyen.3@city.ac.uk wrote: Hi Kai, This is about the issue in the History Map you mentioned a few days ago. Luckily, the code for this part was well commented. Function onNodeClicked https://github.com/Vis4Sense/HistoryMap/blob/master/src/pages/index.js#L439 handles when a node is clicked. You can see in the code two cases are considered: • Found in opening tabs: active the tab, scroll to the captured element (when text/image is highlighted) • Not found: create a new tab and scroll Hope it helps. Cheers, Phong

kaidatavis commented 6 years ago

Hi Phong,

The index.js is now the historyMap.js and the onNodeClicked function is not changed: https://github.com/Vis4Sense/HistoryMap/blob/bffc09f33de3f84c5ddf2af42c6a615e36bdeca1/src/historyMap/historyMap.js#L57

What changed is the tab creation listener in browser.js, which now creates a new historymap node when a new tab is opened in Chrome: https://github.com/Vis4Sense/HistoryMap/blob/bffc09f33de3f84c5ddf2af42c6a615e36bdeca1/src/historyMap/browser.js#L41 This is different from how the tab creation listener works in the old ‘browser.js’, which I assumes stops it from creating duplicate historyMap node (i.e., the current bug). It will be helpful if you can explain how the old tab creation listener works and then this can be added to the new version. https://github.com/Vis4Sense/HistoryMap/blob/230877cea5704aae7501997ac5b8ad4433d8de03/src/js/provenance/browser.js#L39

kaidatavis commented 6 years ago
  1. Add a new attribute to the node object that records whether its tab status is 'opened' or 'closed';
    1. The 'tab status' should be set to 'opened' when a new node is first created;
    2. The 'tab status' should be set to 'closed' when a tab is closed;
  2. Add a second new attribute to the 'node' object that records whether it is 'clicked' in the historyMap
    1. The 'clicked' status is false when the node is first created;
    2. The 'clicked' status becomes true whenever the node is clicked after its creation;
    3. Need to reset the 'clicked' status to 'false' after step 3 below finishes.
  3. When a new tab is created, the onTabCreation function in browser.js should check if
    1. If there is a node with the same url,
    2. If yes, is the tab for this node closed (using the 'tab status' mentioned earlier),
    3. If closed, if this node is just being clicked
    4. If all these conditions are true, this tab is created by clicking a hisotryMap node, therefore no new node for it.
HumzahJavid commented 6 years ago

The partial fix commit above incorporated logic primarily in browser.js to prevent additional nodes being generated and displayed in historyMap page. This used a clicked status (representing the most recently clicked node) and whether the clicked node was a history map node or an annotation (highlight) node.

Whilst the above logic fixed the issue for clicking history map nodes, it was still present for annotation (highlight) nodes.

HumzahJavid commented 6 years ago

The additional nodes were still being created when a highlight node was clicked, the cause was "click-node" nodes being present in historyMap.view.layout.forest().vertices(); these were generated by the following line in redraw function

https://github.com/Vis4Sense/HistoryMap/blob/746e5affd0a6a4b7666697dc592af9c75af608e9/src/historyMap/historyMap.js#L52

 historyMap.view.redraw = function () {
        historyMap.model.tree = listToTree(nodes);
        historyMapView.width(window.innerWidth).height(window.innerHeight);
        d3.select('.sm-history-map-container').datum(historyMap.model.tree).call(historyMapView);
    }

Solution was to add the following line, preventing these nodes from appearing in historyMap.model.tree and in ...forest().vertices() https://github.com/Vis4Sense/HistoryMap/blob/746e5affd0a6a4b7666697dc592af9c75af608e9/src/historyMap/listToTree.js#L63