{"action":"created","comment":{"url":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/comments/70278632","id":70278632,"diff_hunk":"@@ -411,33 +398,28 @@ void DiffManager::showDiff(QString oldVersion, QString newVersion)\n \tdiffViewItem->setZoomLabelsEnabled(false);\n \n \tint row = 0;\n-\tauto diffFrames = createDiffFrames(diffSetup, changedNodesToVisualize, changesWithNodes);\n-\tfor (auto diffFrame : diffFrames)\n+\tfor (auto diffFrame : computeDiffFramesAndOverlays(oldVersion, newVersion, diffViewItem))\n \t\tdiffViewItem->insertNode(diffFrame, {row++, 0});\n \n-\tcreateOverlaysForChanges(changesWithNodes, diffViewItem, diffSetup, diffFrames.first());\n-\n \t// switch to the newly created view\n \tVisualization::VisualizationManager::instance().mainScene()->viewItems()->switchToView(diffViewItem);\n }\n \n void DiffManager::createOverlaysForChanges(QList<ChangeWithNodes> changesWithNodes, Visualization::ViewItem* viewItem,\n-\t\t\t\t\t\t\t\t\t\t\t\t\t\t DiffSetup diffSetup, VersionControlUI::DiffFrame* diffFrame)\n+\t\t\t\t\t\t\t\t\t\t\t\t\t\t DiffSetup diffSetup, Visualization::Item* ankerItem)","path":"VersionControlUI/src/DiffManager.cpp","position":null,"original_position":37,"commit_id":"c03e65a6aab82edce2c99ade1dd590aa36a6d912","original_commit_id":"c03e65a6aab82edce2c99ade1dd590aa36a6d912","user":{"login":"dimitar-asenov","id":1152976,"avatar_url":"https://avatars.githubusercontent.com/u/1152976?v=3","gravatar_id":"","url":"https://api.github.com/users/dimitar-asenov","html_url":"https://github.com/dimitar-asenov","followers_url":"https://api.github.com/users/dimitar-asenov/followers","following_url":"https://api.github.com/users/dimitar-asenov/following{/other_user}","gists_url":"https://api.github.com/users/dimitar-asenov/gists{/gist_id}","starred_url":"https://api.github.com/users/dimitar-asenov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dimitar-asenov/subscriptions","organizations_url":"https://api.github.com/users/dimitar-asenov/orgs","repos_url":"https://api.github.com/users/dimitar-asenov/repos","events_url":"https://api.github.com/users/dimitar-asenov/events{/privacy}","received_events_url":"https://api.github.com/users/dimitar-asenov/received_events","type":"User","site_admin":false},"body":"Do you mean `anchorItem`?","created_at":"2016-07-11T15:21:05Z","updated_at":"2016-07-11T15:21:05Z","html_url":"https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70278632","pull_request_url":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/415","_links":{"self":{"href":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/comments/70278632"},"html":{"href":"https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70278632"},"pull_request":{"href":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/415"}}},"pull_request":{"url":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/415","id":76926615,"html_url":"https://github.com/dimitar-asenov/Envision/pull/415","diff_url":"https://github.com/dimitar-asenov/Envision/pull/415.diff","patch_url":"https://github.com/dimitar-asenov/Envision/pull/415.patch","issue_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/415","number":415,"state":"open","locked":false,"title":"Gm automated ordering","user":{"login":"mgalbier","id":17177211,"avatar_url":"https://avatars.githubusercontent.com/u/17177211?v=3","gravatar_id":"","url":"https://api.github.com/users/mgalbier","html_url":"https://github.com/mgalbier","followers_url":"https://api.github.com/users/mgalbier/followers","following_url":"https://api.github.com/users/mgalbier/following{/other_user}","gists_url":"https://api.github.com/users/mgalbier/gists{/gist_id}","starred_url":"https://api.github.com/users/mgalbier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mgalbier/subscriptions","organizations_url":"https://api.github.com/users/mgalbier/orgs","repos_url":"https://api.github.com/users/mgalbier/repos","events_url":"https://api.github.com/users/mgalbier/events{/privacy}","received_events_url":"https://api.github.com/users/mgalbier/received_events","type":"User","site_admin":false},"body":"- Rename DiffComparisonPair to DiffFrame\r\n- Register CClear on HSceneHandlerItem\r\n- Register CHistory on HSceneHandlerItem\r\n - Use source instead of target in implementation, since target is now SceneHandlerItem\r\n- Register CDiff on HSceneHandlerItem\r\n - Use source instead of target in implementation, since target is now SceneHandlerItem\r\n- Refactor DiffManager in preparation for changes ordering extension\r\n- Initial commit for automated changes ordering\r\n<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70231522%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70231593%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70231940%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70232839%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70233084%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70233331%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70233506%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70233533%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70233627%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70233789%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70245426%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70245527%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70245632%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70245673%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70245750%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70245889%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70245929%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70246031%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70246227%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70246533%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70246681%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70246952%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247334%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247418%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247657%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247733%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247763%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247874%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247953%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70248110%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70248177%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70248262%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23issuecomment-231721073%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70248443%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70248511%22%5D%2C%20%22comments%22%3A%20%7B%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23issuecomment-231721073%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%27m%20done%20with%20the%20review.%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A33%3A06Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22General%20Comment%22%7D%2C%20%22Pull%20d30495e7f1346c97fa7d42d437f88663c75994a7%20VersionControlUI/src/nodes/DiffFrame.h%2042%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70233331%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22In%20the%20future%2C%20please%20do%20not%20sneak%20in%20changes%20that%20do%20not%20correspond%20to%20the%20description%20of%20the%20commit/PR.%22%2C%20%22created_at%22%3A%20%222016-07-11T10%3A19%3A19Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20realize%20this%20was%20most%20likely%20accidental.%20Just%20be%20more%20careful.%5Cr%5Cn%5Cr%5CnTo%20make%20this%20easier%2C%20it%20helps%20to%20make/edit%20local%20commits%20more%20often%2C%20and%20then%20just%20submit%20them%2C%20once%20you%27re%20happy%20with%20the%20local%20state.%22%2C%20%22created_at%22%3A%20%222016-07-11T10%3A20%3A46Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22Yes%20it%20was%20accidental%2C%20since%20the%20rename%20just%20shows%20the%20whole%20file%20as%20new.%20%5Cr%5Cn%5Cr%5CnOK%2C%20I%27ll%20try%20to%20do%20this.%22%2C%20%22created_at%22%3A%20%222016-07-11T10%3A23%3A05Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/17177211%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/mgalbier%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20VersionControlUI/src/nodes/DiffFrame.h%3AL66-96%22%7D%2C%20%22Pull%20552b3621c29e787fac85d1e3ac474df1d1acb76a%20VersionControlUI/src/DiffManager.cpp%2015%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70231940%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Good.%20Methods%20should%20indeed%20be%20as%20focused%20as%20possible%20%28and%20therefore%20small%29.%22%2C%20%22created_at%22%3A%20%222016-07-11T10%3A07%3A56Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20VersionControlUI/src/DiffManager.cpp%3AL411-474%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%2087%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247334%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22%60diffCompPair%60%20%3F%3F%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A24%3A53Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%3AL1-155%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/src/CodeReviewManager.h%2012%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70245426%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Hmm%2C%20I%27m%20not%20sure%20that%20introducing%20these%20special%20names%20improves%20things.%20What%20was%20your%20reason%20for%20introducing%20them%3F%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A08%3A14Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22During%20the%20meeting%20we%20agreed%20to%20introduce%20some%20special%20names%20to%20avoid%20getting%20the%20method%20signatures%20too%20confusing.%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A11%3A11Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/17177211%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/mgalbier%22%7D%7D%2C%20%7B%22body%22%3A%20%22We%20did%3F%20I%20do%20not%20remember%20that%20particular%20discussion.%20Perhaps%20there%20was%20a%20misunderstanding.%20When%20I%20talked%20about%20different%20types%2C%20I%20meant%20the%20names%20of%20the%20individual%20strategies.%5Cr%5Cn%5Cr%5CnI%20don%27t%20feel%20like%20naming%20a%20%60QList%3CDiffFrame%2A%3E%60%20in%20a%20few%20different%20ways%20makes%20any%20difference%20here.%20Do%20you%20think%20it%20helps%20in%20some%20way%3F%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A14%3A59Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%27m%20not%20sure%2C%20it%20helps%20to%20quickly%20see%20what%20a%20method%20is%20actually%20returning%20%28e.g.%20%60OrderedDiffFrames%20Orderings%3A%3AalphabeticalOrdering%28DiffFrames%20diffFrames%29%60%29%20but%20on%20the%20other%20hand%20the%20method%20name%20should%20be%20clear%20enough.%5Cr%5Cn%5Cr%5CnI%20can%20remove%20the%20using%20definitons%20for%20the%20lists%20and%20leave%20the%20ones%20for%20the%20functions%2C%20what%20do%20you%20think%3F%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A18%3A48Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/17177211%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/mgalbier%22%7D%7D%2C%20%7B%22body%22%3A%20%22Yes%2C%20that%20sounds%20good.%5Cr%5Cn%5Cr%5CnFunctions%20should%20definitely%20have%20names%20defined%20with%20%60using%60%2C%20but%20for%20the%20other%20types%20here%2C%20that%20seems%20unnecessary.%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A35%3A00Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/src/CodeReviewManager.h%3AL28-69%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/src/CodeReviewManager.h%2037%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70245527%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Why%20do%20we%20need%20this%3F%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A09%3A05Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22We%20don%27t%20need%20this.%20You%20passed%20the%20test%21%20%5Cr%5Cn%5Cr%5Cn%28Should%20not%20be%20in%20there%2C%20I%27ll%20remove%20it%29%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A12%3A40Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/17177211%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/mgalbier%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/src/CodeReviewManager.h%3AL28-69%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/src/commands/CCodeReview.cpp%2042%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70245889%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22In%20general%20when%20writing%20the%20include%20section%3A%5Cr%5Cn-%20Group%20includes%20by%20plug-in.%5Cr%5Cn-%20Put%20the%20includes%20of%20the%20current%20plug-in%20first%20and%20then%20all%20the%20rest.%5Cr%5Cn-%20Use%20a%20single%20blank%20line%20to%20separate%20the%20sections%20with%20the%20different%20plug-ins.%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A12%3A18Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22OK%2C%20will%20do%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A13%3A31Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/17177211%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/mgalbier%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/src/commands/CCodeReview.cpp%3AL1-119%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/CMakeLists.txt%2013%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70248262%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22So%20I%27m%20not%20convinced%20about%20the%20the%20names%20of%20these%20classes%20and%20the%20directories%20they%20are%20in%2C%20but%20we%20can%20adjust%20this%20later%2C%20once%20we%20have%20more%20strategies.%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A32%3A51Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/CMakeLists.txt%3AL21-34%22%7D%2C%20%22Pull%204918a677f6d04ffea99d25cd20b00ecff179f53d%20VersionControlUI/src/commands/CDiff.cpp%2011%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70231593%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22This%20is%20the%20kind%20of%20check%20that%20should%20also%20be%20there%20for%20the%20other%20command.%22%2C%20%22created_at%22%3A%20%222016-07-11T10%3A05%3A25Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20VersionControlUI/src/commands/CDiff.cpp%3AL44-58%22%7D%2C%20%22Pull%20552b3621c29e787fac85d1e3ac474df1d1acb76a%20VersionControlUI/src/DiffManager.h%2024%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70233627%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Comments%20%3D%3D%20good%22%2C%20%22created_at%22%3A%20%222016-07-11T10%3A21%3A34Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20VersionControlUI/src/DiffManager.h%3AL155-172%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/CMakeLists.txt%204%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70245632%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Is%20this%20a%20new%20command%20or%20did%20you%20just%20move%20it%20here%3F%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A10%3A04Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22It%20is%20a%20new%20command.%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A10%3A27Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/17177211%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/mgalbier%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/CMakeLists.txt%3AL10-17%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%2098%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247763%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22%60%3AdiffPairs%60%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A28%3A18Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%3AL1-155%22%7D%2C%20%22Pull%20c48b123f966a8d7454553605f1ba4d5c19897326%20VersionControlUI/src/commands/CHistory.cpp%2010%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70231522%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22This%20could%20return%20%60nullptr%60%2C%20in%20which%20case%20you%20should%20make%20sure%20not%20to%20do%20anything.%20Ideally%2C%20this%20is%20caught%20already%20in%20the%20%60canExecute%60%20method.%22%2C%20%22created_at%22%3A%20%222016-07-11T10%3A04%3A59Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20VersionControlUI/src/commands/CHistory.cpp%3AL41-54%22%7D%2C%20%22Pull%20552b3621c29e787fac85d1e3ac474df1d1acb76a%20VersionControlUI/src/DiffManager.cpp%2050%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70232839%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Somehow%20the%20name%20of%20this%20method%20does%20not%20reflect%20what%27s%20going%20on.%20The%20body%20seems%20to%20do%20a%20lot%20more%20that%20just%20%5C%22compute%5C%22%2C%20for%20example%20it%20creates%20new%20version%20managers%20and%20registers%20them%20on%20the%20scene.%5Cr%5Cn%5Cr%5CnAlso%20do%20you%20mean%20%60with%60%20or%20%60and%60%20in%20the%20method%20name%3F%20%60With%60%20implies%20that%20this%20method%20computes%20only%20those%20DiffFrames%20that%20have%20an%20Overlay.%20%60And%60%20implies%20that%20it%27s%20doing%20both.%20It%20seems%20like%20the%20second%20is%20the%20case%20here.%5Cr%5Cn%5Cr%5CnThere%20are%20quite%20a%20few%20methods%20with%20similar%20names%20and%20we%20need%20to%20make%20it%20clearer%20what%20method%20does%20what%3A%5Cr%5Cn%60computeDiffFramesWithOverlays%60%2C%20%60computeDiff%60%2C%20createDiffFrames%60.%22%2C%20%22created_at%22%3A%20%222016-07-11T10%3A15%3A09Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20VersionControlUI/src/DiffManager.cpp%3AL411-474%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%20110%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247953%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Please%20put%20the%20%60else%60%20on%20a%20new%20line%20by%20itself.%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A29%3A58Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%3AL1-155%22%7D%2C%20%22Pull%20552b3621c29e787fac85d1e3ac474df1d1acb76a%20VersionControlUI/src/DiffManager.cpp%2099%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70233084%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%20see%20you%20introduced%20this%20new%20argument%20here%20%60changesWithNodes%60.%20Could%20you%20remind%20me%20what%20this%20was%20for%3F%20Maybe%20even%20put%20a%20brief%20comment%20here%20before%20the%20loop.%22%2C%20%22created_at%22%3A%20%222016-07-11T10%3A17%3A15Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22This%20is%20used%20to%20decide%20which%20changed%20nodes%20should%20be%20stored%20in%20the%20lists%20of%20%60DiffFrame%60.%20It%20knows%20now%20which%20changed%20nodes%2C%20in%20which%20version%20it%20contains.%20%22%2C%20%22created_at%22%3A%20%222016-07-11T10%3A20%3A34Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/17177211%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/mgalbier%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20VersionControlUI/src/DiffManager.cpp%3AL843-859%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%2068%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70248110%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22what%20does%20this%20do%3F%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A31%3A28Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22oh%20nevremind%2C%20I%20saw%20it.%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A32%3A03Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22remembers%20all%20the%20nodes%20which%20are%20shown%20in%20this%20DiffFrame%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A34%3A25Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/17177211%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/mgalbier%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%3AL1-155%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%2080%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247657%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22the%20argument%20name%20should%20end%20in%20an%20%60s%60%20shouldn%27t%20it%3F%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A27%3A34Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22Yes%20and%20I%20have%20to%20go%20over%20the%20renamings%20in%20this%20file%20again.%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A29%3A16Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/17177211%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/mgalbier%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%3AL1-155%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/src/groupings/UseAnalysisGroupings.h%2049%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70246533%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22shouldn%27t%20some%20of%20these%20methods%20be%20private%3F%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A17%3A36Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22Yes%20you%20are%20right.%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A21%3A11Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/17177211%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/mgalbier%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/src/groupings/UseAnalysisGroupings.h%3AL1-57%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%2094%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247733%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22isn%27t%20just%20%60auto%20diffPair%3A%20diffPairs%60%20better%3F%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A28%3A03Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%3AL1-155%22%7D%2C%20%22Pull%204957689ddcec3694e5042a1907eea36a74749055%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%2084%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/415%23discussion_r70247418%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22better%20call%20this%20%60diffFrameToInfo%60%22%2C%20%22created_at%22%3A%20%222016-07-11T12%3A25%3A42Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20CodeReview/src/groupings/UseAnalysisGroupings.cpp%3AL1-155%22%7D%7D%7D'></a>\r\n<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>\r\n- [x] <a href='#crh-comment-Pull c48b123f966a8d7454553605f1ba4d5c19897326 VersionControlUI/src/commands/CHistory.cpp 10'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70231522'>File: VersionControlUI/src/commands/CHistory.cpp:L41-54</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> This could return `nullptr`, in which case you should make sure not to do anything. Ideally, this is caught already in the `canExecute` method.\r\n- [x] <a href='#crh-comment-Pull 4918a677f6d04ffea99d25cd20b00ecff179f53d VersionControlUI/src/commands/CDiff.cpp 11'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70231593'>File: VersionControlUI/src/commands/CDiff.cpp:L44-58</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> This is the kind of check that should also be there for the other command.\r\n- [x] <a href='#crh-comment-Pull 552b3621c29e787fac85d1e3ac474df1d1acb76a VersionControlUI/src/DiffManager.cpp 15'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70231940'>File: VersionControlUI/src/DiffManager.cpp:L411-474</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Good. Methods should indeed be as focused as possible (and therefore small).\r\n- [x] <a href='#crh-comment-Pull 552b3621c29e787fac85d1e3ac474df1d1acb76a VersionControlUI/src/DiffManager.cpp 50'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70232839'>File: VersionControlUI/src/DiffManager.cpp:L411-474</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Somehow the name of this method does not reflect what's going on. The body seems to do a lot more that just \"compute\", for example it creates new version managers and registers them on the scene.\r\nAlso do you mean `with` or `and` in the method name? `With` implies that this method computes only those DiffFrames that have an Overlay. `And` implies that it's doing both. It seems like the second is the case here.\r\nThere are quite a few methods with similar names and we need to make it clearer what method does what:\r\n`computeDiffFramesWithOverlays`, `computeDiff`, createDiffFrames`.\r\n- [x] <a href='#crh-comment-Pull 552b3621c29e787fac85d1e3ac474df1d1acb76a VersionControlUI/src/DiffManager.cpp 99'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70233084'>File: VersionControlUI/src/DiffManager.cpp:L843-859</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> I see you introduced this new argument here `changesWithNodes`. Could you remind me what this was for? Maybe even put a brief comment here before the loop.\r\n- <a href='https://github.com/mgalbier'><img border=0 src='https://avatars.githubusercontent.com/u/17177211?v=3' height=16 width=16'></a> This is used to decide which changed nodes should be stored in the lists of `DiffFrame`. It knows now which changed nodes, in which version it contains.\r\n- [x] <a href='#crh-comment-Pull d30495e7f1346c97fa7d42d437f88663c75994a7 VersionControlUI/src/nodes/DiffFrame.h 42'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70233331'>File: VersionControlUI/src/nodes/DiffFrame.h:L66-96</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> In the future, please do not sneak in changes that do not correspond to the description of the commit/PR.\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> I realize this was most likely accidental. Just be more careful.\r\nTo make this easier, it helps to make/edit local commits more often, and then just submit them, once you're happy with the local state.\r\n- <a href='https://github.com/mgalbier'><img border=0 src='https://avatars.githubusercontent.com/u/17177211?v=3' height=16 width=16'></a> Yes it was accidental, since the rename just shows the whole file as new.\r\nOK, I'll try to do this.\r\n- [x] <a href='#crh-comment-Pull 552b3621c29e787fac85d1e3ac474df1d1acb76a VersionControlUI/src/DiffManager.h 24'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70233627'>File: VersionControlUI/src/DiffManager.h:L155-172</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Comments == good\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/src/CodeReviewManager.h 12'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70245426'>File: CodeReview/src/CodeReviewManager.h:L28-69</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Hmm, I'm not sure that introducing these special names improves things. What was your reason for introducing them?\r\n- <a href='https://github.com/mgalbier'><img border=0 src='https://avatars.githubusercontent.com/u/17177211?v=3' height=16 width=16'></a> During the meeting we agreed to introduce some special names to avoid getting the method signatures too confusing.\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> We did? I do not remember that particular discussion. Perhaps there was a misunderstanding. When I talked about different types, I meant the names of the individual strategies.\r\nI don't feel like naming a `QList<DiffFrame*>` in a few different ways makes any difference here. Do you think it helps in some way?\r\n- <a href='https://github.com/mgalbier'><img border=0 src='https://avatars.githubusercontent.com/u/17177211?v=3' height=16 width=16'></a> I'm not sure, it helps to quickly see what a method is actually returning (e.g. `OrderedDiffFrames Orderings::alphabeticalOrdering(DiffFrames diffFrames)`) but on the other hand the method name should be clear enough.\r\nI can remove the using definitons for the lists and leave the ones for the functions, what do you think?\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Yes, that sounds good.\r\nFunctions should definitely have names defined with `using`, but for the other types here, that seems unnecessary.\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/src/CodeReviewManager.h 37'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70245527'>File: CodeReview/src/CodeReviewManager.h:L28-69</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Why do we need this?\r\n- <a href='https://github.com/mgalbier'><img border=0 src='https://avatars.githubusercontent.com/u/17177211?v=3' height=16 width=16'></a> We don't need this. You passed the test!\r\n(Should not be in there, I'll remove it)\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/CMakeLists.txt 4'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70245632'>File: CodeReview/CMakeLists.txt:L10-17</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Is this a new command or did you just move it here?\r\n- <a href='https://github.com/mgalbier'><img border=0 src='https://avatars.githubusercontent.com/u/17177211?v=3' height=16 width=16'></a> It is a new command.\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/src/commands/CCodeReview.cpp 42'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70245889'>File: CodeReview/src/commands/CCodeReview.cpp:L1-119</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> In general when writing the include section:\r\n- Group includes by plug-in.\r\n- Put the includes of the current plug-in first and then all the rest.\r\n- Use a single blank line to separate the sections with the different plug-ins.\r\n- <a href='https://github.com/mgalbier'><img border=0 src='https://avatars.githubusercontent.com/u/17177211?v=3' height=16 width=16'></a> OK, will do\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/src/groupings/UseAnalysisGroupings.h 49'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70246533'>File: CodeReview/src/groupings/UseAnalysisGroupings.h:L1-57</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> shouldn't some of these methods be private?\r\n- <a href='https://github.com/mgalbier'><img border=0 src='https://avatars.githubusercontent.com/u/17177211?v=3' height=16 width=16'></a> Yes you are right.\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/src/groupings/UseAnalysisGroupings.cpp 87'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70247334'>File: CodeReview/src/groupings/UseAnalysisGroupings.cpp:L1-155</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> `diffCompPair` ??\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/src/groupings/UseAnalysisGroupings.cpp 84'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70247418'>File: CodeReview/src/groupings/UseAnalysisGroupings.cpp:L1-155</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> better call this `diffFrameToInfo`\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/src/groupings/UseAnalysisGroupings.cpp 80'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70247657'>File: CodeReview/src/groupings/UseAnalysisGroupings.cpp:L1-155</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> the argument name should end in an `s` shouldn't it?\r\n- <a href='https://github.com/mgalbier'><img border=0 src='https://avatars.githubusercontent.com/u/17177211?v=3' height=16 width=16'></a> Yes and I have to go over the renamings in this file again.\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/src/groupings/UseAnalysisGroupings.cpp 94'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70247733'>File: CodeReview/src/groupings/UseAnalysisGroupings.cpp:L1-155</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> isn't just `auto diffPair: diffPairs` better?\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/src/groupings/UseAnalysisGroupings.cpp 98'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70247763'>File: CodeReview/src/groupings/UseAnalysisGroupings.cpp:L1-155</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> `:diffPairs`\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/src/groupings/UseAnalysisGroupings.cpp 110'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70247953'>File: CodeReview/src/groupings/UseAnalysisGroupings.cpp:L1-155</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Please put the `else` on a new line by itself.\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/src/groupings/UseAnalysisGroupings.cpp 68'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70248110'>File: CodeReview/src/groupings/UseAnalysisGroupings.cpp:L1-155</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> what does this do?\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> oh nevremind, I saw it.\r\n- <a href='https://github.com/mgalbier'><img border=0 src='https://avatars.githubusercontent.com/u/17177211?v=3' height=16 width=16'></a> remembers all the nodes which are shown in this DiffFrame\r\n- [x] <a href='#crh-comment-Pull 4957689ddcec3694e5042a1907eea36a74749055 CodeReview/CMakeLists.txt 13'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#discussion_r70248262'>File: CodeReview/CMakeLists.txt:L21-34</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> So I'm not convinced about the the names of these classes and the directories they are in, but we can adjust this later, once we have more strategies.\r\n- [x] <a href='#crh-comment-General Comment'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/415#issuecomment-231721073'>General Comment</a></b>\r\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> I'm done with the review.\r\n\r\n\r\n<a href='https://www.codereviewhub.com/dimitar-asenov/Envision/pull/415?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/dimitar-asenov/Envision/pull/415?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://github.com/dimitar-asenov/Envision/pull/415'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>\r\n<a href='#crh-end'></a>","created_at":"2016-07-11T09:52:16Z","updated_at":"2016-07-11T15:21:05Z","closed_at":null,"merged_at":null,"merge_commit_sha":"7276cce4806e3904e6305fffe0bf6f593fc0ad81","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/415/commits","review_comments_url":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/415/comments","review_comment_url":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/comments{/number}","comments_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/415/comments","statuses_url":"https://api.github.com/repos/dimitar-asenov/Envision/statuses/c03e65a6aab82edce2c99ade1dd590aa36a6d912","head":{"label":"mgalbier:gm-automated-ordering","ref":"gm-automated-ordering","sha":"c03e65a6aab82edce2c99ade1dd590aa36a6d912","user":{"login":"mgalbier","id":17177211,"avatar_url":"https://avatars.githubusercontent.com/u/17177211?v=3","gravatar_id":"","url":"https://api.github.com/users/mgalbier","html_url":"https://github.com/mgalbier","followers_url":"https://api.github.com/users/mgalbier/followers","following_url":"https://api.github.com/users/mgalbier/following{/other_user}","gists_url":"https://api.github.com/users/mgalbier/gists{/gist_id}","starred_url":"https://api.github.com/users/mgalbier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mgalbier/subscriptions","organizations_url":"https://api.github.com/users/mgalbier/orgs","repos_url":"https://api.github.com/users/mgalbier/repos","events_url":"https://api.github.com/users/mgalbier/events{/privacy}","received_events_url":"https://api.github.com/users/mgalbier/received_events","type":"User","site_admin":false},"repo":{"id":53584127,"name":"Envision","full_name":"mgalbier/Envision","owner":{"login":"mgalbier","id":17177211,"avatar_url":"https://avatars.githubusercontent.com/u/17177211?v=3","gravatar_id":"","url":"https://api.github.com/users/mgalbier","html_url":"https://github.com/mgalbier","followers_url":"https://api.github.com/users/mgalbier/followers","following_url":"https://api.github.com/users/mgalbier/following{/other_user}","gists_url":"https://api.github.com/users/mgalbier/gists{/gist_id}","starred_url":"https://api.github.com/users/mgalbier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mgalbier/subscriptions","organizations_url":"https://api.github.com/users/mgalbier/orgs","repos_url":"https://api.github.com/users/mgalbier/repos","events_url":"https://api.github.com/users/mgalbier/events{/privacy}","received_events_url":"https://api.github.com/users/mgalbier/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/mgalbier/Envision","description":"A next generation IDE with a visual programming interface","fork":true,"url":"https://api.github.com/repos/mgalbier/Envision","forks_url":"https://api.github.com/repos/mgalbier/Envision/forks","keys_url":"https://api.github.com/repos/mgalbier/Envision/keys{/key_id}","collaborators_url":"https://api.github.com/repos/mgalbier/Envision/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/mgalbier/Envision/teams","hooks_url":"https://api.github.com/repos/mgalbier/Envision/hooks","issue_events_url":"https://api.github.com/repos/mgalbier/Envision/issues/events{/number}","events_url":"https://api.github.com/repos/mgalbier/Envision/events","assignees_url":"https://api.github.com/repos/mgalbier/Envision/assignees{/user}","branches_url":"https://api.github.com/repos/mgalbier/Envision/branches{/branch}","tags_url":"https://api.github.com/repos/mgalbier/Envision/tags","blobs_url":"https://api.github.com/repos/mgalbier/Envision/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/mgalbier/Envision/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/mgalbier/Envision/git/refs{/sha}","trees_url":"https://api.github.com/repos/mgalbier/Envision/git/trees{/sha}","statuses_url":"https://api.github.com/repos/mgalbier/Envision/statuses/{sha}","languages_url":"https://api.github.com/repos/mgalbier/Envision/languages","stargazers_url":"https://api.github.com/repos/mgalbier/Envision/stargazers","contributors_url":"https://api.github.com/repos/mgalbier/Envision/contributors","subscribers_url":"https://api.github.com/repos/mgalbier/Envision/subscribers","subscription_url":"https://api.github.com/repos/mgalbier/Envision/subscription","commits_url":"https://api.github.com/repos/mgalbier/Envision/commits{/sha}","git_commits_url":"https://api.github.com/repos/mgalbier/Envision/git/commits{/sha}","comments_url":"https://api.github.com/repos/mgalbier/Envision/comments{/number}","issue_comment_url":"https://api.github.com/repos/mgalbier/Envision/issues/comments{/number}","contents_url":"https://api.github.com/repos/mgalbier/Envision/contents/{+path}","compare_url":"https://api.github.com/repos/mgalbier/Envision/compare/{base}...{head}","merges_url":"https://api.github.com/repos/mgalbier/Envision/merges","archive_url":"https://api.github.com/repos/mgalbier/Envision/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/mgalbier/Envision/downloads","issues_url":"https://api.github.com/repos/mgalbier/Envision/issues{/number}","pulls_url":"https://api.github.com/repos/mgalbier/Envision/pulls{/number}","milestones_url":"https://api.github.com/repos/mgalbier/Envision/milestones{/number}","notifications_url":"https://api.github.com/repos/mgalbier/Envision/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/mgalbier/Envision/labels{/name}","releases_url":"https://api.github.com/repos/mgalbier/Envision/releases{/id}","deployments_url":"https://api.github.com/repos/mgalbier/Envision/deployments","created_at":"2016-03-10T12:51:01Z","updated_at":"2016-03-10T12:51:08Z","pushed_at":"2016-07-11T15:07:49Z","git_url":"git://github.com/mgalbier/Envision.git","ssh_url":"git@github.com:mgalbier/Envision.git","clone_url":"https://github.com/mgalbier/Envision.git","svn_url":"https://github.com/mgalbier/Envision","homepage":"http://dimitar-asenov.github.com/Envision/","size":64792,"stargazers_count":0,"watchers_count":0,"language":"C++","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"development"}},"base":{"label":"dimitar-asenov:development","ref":"development","sha":"47fd61aa090dc4b5dd03312bc9b49bf3b6ccf1df","user":{"login":"dimitar-asenov","id":1152976,"avatar_url":"https://avatars.githubusercontent.com/u/1152976?v=3","gravatar_id":"","url":"https://api.github.com/users/dimitar-asenov","html_url":"https://github.com/dimitar-asenov","followers_url":"https://api.github.com/users/dimitar-asenov/followers","following_url":"https://api.github.com/users/dimitar-asenov/following{/other_user}","gists_url":"https://api.github.com/users/dimitar-asenov/gists{/gist_id}","starred_url":"https://api.github.com/users/dimitar-asenov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dimitar-asenov/subscriptions","organizations_url":"https://api.github.com/users/dimitar-asenov/orgs","repos_url":"https://api.github.com/users/dimitar-asenov/repos","events_url":"https://api.github.com/users/dimitar-asenov/events{/privacy}","received_events_url":"https://api.github.com/users/dimitar-asenov/received_events","type":"User","site_admin":false},"repo":{"id":2650576,"name":"Envision","full_name":"dimitar-asenov/Envision","owner":{"login":"dimitar-asenov","id":1152976,"avatar_url":"https://avatars.githubusercontent.com/u/1152976?v=3","gravatar_id":"","url":"https://api.github.com/users/dimitar-asenov","html_url":"https://github.com/dimitar-asenov","followers_url":"https://api.github.com/users/dimitar-asenov/followers","following_url":"https://api.github.com/users/dimitar-asenov/following{/other_user}","gists_url":"https://api.github.com/users/dimitar-asenov/gists{/gist_id}","starred_url":"https://api.github.com/users/dimitar-asenov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dimitar-asenov/subscriptions","organizations_url":"https://api.github.com/users/dimitar-asenov/orgs","repos_url":"https://api.github.com/users/dimitar-asenov/repos","events_url":"https://api.github.com/users/dimitar-asenov/events{/privacy}","received_events_url":"https://api.github.com/users/dimitar-asenov/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/dimitar-asenov/Envision","description":"A next generation IDE with a visual programming interface","fork":false,"url":"https://api.github.com/repos/dimitar-asenov/Envision","forks_url":"https://api.github.com/repos/dimitar-asenov/Envision/forks","keys_url":"https://api.github.com/repos/dimitar-asenov/Envision/keys{/key_id}","collaborators_url":"https://api.github.com/repos/dimitar-asenov/Envision/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/dimitar-asenov/Envision/teams","hooks_url":"https://api.github.com/repos/dimitar-asenov/Envision/hooks","issue_events_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/events{/number}","events_url":"https://api.github.com/repos/dimitar-asenov/Envision/events","assignees_url":"https://api.github.com/repos/dimitar-asenov/Envision/assignees{/user}","branches_url":"https://api.github.com/repos/dimitar-asenov/Envision/branches{/branch}","tags_url":"https://api.github.com/repos/dimitar-asenov/Envision/tags","blobs_url":"https://api.github.com/repos/dimitar-asenov/Envision/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/dimitar-asenov/Envision/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/dimitar-asenov/Envision/git/refs{/sha}","trees_url":"https://api.github.com/repos/dimitar-asenov/Envision/git/trees{/sha}","statuses_url":"https://api.github.com/repos/dimitar-asenov/Envision/statuses/{sha}","languages_url":"https://api.github.com/repos/dimitar-asenov/Envision/languages","stargazers_url":"https://api.github.com/repos/dimitar-asenov/Envision/stargazers","contributors_url":"https://api.github.com/repos/dimitar-asenov/Envision/contributors","subscribers_url":"https://api.github.com/repos/dimitar-asenov/Envision/subscribers","subscription_url":"https://api.github.com/repos/dimitar-asenov/Envision/subscription","commits_url":"https://api.github.com/repos/dimitar-asenov/Envision/commits{/sha}","git_commits_url":"https://api.github.com/repos/dimitar-asenov/Envision/git/commits{/sha}","comments_url":"https://api.github.com/repos/dimitar-asenov/Envision/comments{/number}","issue_comment_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/comments{/number}","contents_url":"https://api.github.com/repos/dimitar-asenov/Envision/contents/{+path}","compare_url":"https://api.github.com/repos/dimitar-asenov/Envision/compare/{base}...{head}","merges_url":"https://api.github.com/repos/dimitar-asenov/Envision/merges","archive_url":"https://api.github.com/repos/dimitar-asenov/Envision/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/dimitar-asenov/Envision/downloads","issues_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues{/number}","pulls_url":"https://api.github.com/repos/dimitar-asenov/Envision/pulls{/number}","milestones_url":"https://api.github.com/repos/dimitar-asenov/Envision/milestones{/number}","notifications_url":"https://api.github.com/repos/dimitar-asenov/Envision/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/dimitar-asenov/Envision/labels{/name}","releases_url":"https://api.github.com/repos/dimitar-asenov/Envision/releases{/id}","deployments_url":"https://api.github.com/repos/dimitar-asenov/Envision/deployments","created_at":"2011-10-26T12:03:58Z","updated_at":"2016-05-29T16:14:32Z","pushed_at":"2016-07-11T15:07:50Z","git_url":"git://github.com/dimitar-asenov/Envision.git","ssh_url":"git@github.com:dimitar-asenov/Envision.git","clone_url":"https://github.com/dimitar-asenov/Envision.git","svn_url":"https://github.com/dimitar-asenov/Envision","homepage":"http://dimitar-asenov.github.com/Envision/","size":69504,"stargazers_count":28,"watchers_count":28,"language":"C++","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":true,"forks_count":18,"mirror_url":null,"open_issues_count":30,"forks":18,"open_issues":30,"watchers":28,"default_branch":"development"}},"_links":{"self":{"href":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/415"},"html":{"href":"https://github.com/dimitar-asenov/Envision/pull/415"},"issue":{"href":"https://api.github.com/repos/dimitar-asenov/Envision/issues/415"},"comments":{"href":"https://api.github.com/repos/dimitar-asenov/Envision/issues/415/comments"},"review_comments":{"href":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/415/comments"},"review_comment":{"href":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/415/commits"},"statuses":{"href":"https://api.github.com/repos/dimitar-asenov/Envision/statuses/c03e65a6aab82edce2c99ade1dd590aa36a6d912"}}}}
{"actor":{"display_login":"dimitar-asenov"}}