Closed raducoravu closed 3 years ago
Code review part 2: OnGoingReferencesTreeCellRenderer->IncomingReferencesTreeCellRenderer OutgoingReferencesPanel->IncomingReferencesPanel
Maybe in the "com.oxygenxml.ditareferences.tree.references" package we can make 2 subpackages "incoming" (references from other maps and topics to the current opened topic) and "outgoing" (references from the current topic to other resources).
Code review, part 3:
The entire functionality of the "Refresh" action in the "SideViewComponent" in my opinion pertains only to the IncomingReferencesPanel, so why not move there the timer task and all the functionality of "com.oxygenxml.ditareferences.sideview.SideViewComponent.load(boolean, int)"? Maybe add a method "IncomingReferencesPanel.getRefreshAction()" and mount the refresh action from the incoming references tree in the parent panel. On the comparator "com.oxygenxml.ditareferences.tree.references.incoming.IncomingReferencesPanel.searchOngoingRef(URL)" why are you comparing the messages instead of comparing the file names obtained from the system IDs? The messages may change in the future.
"com.oxygenxml.ditareferences.tree.references.incoming.IncomingReferencesPanel.selectRange(WSEditorPage, DocumentPositionedInfo)" should be called on the AWT thread, not on the timer thread.
Maybe you can also install the NVDA text to speech narrator and check your side view for accessibility problems: https://www.oxygenxml.com/doc/versions/22.1/ug-editor/topics/accessibility.html?hl=nvda%2Cnarrator#accessibility__section_r3c_fzn_j3b
A blind person using the side view would first show it using the main menu (and they are using only the keyboard, not the mouse). Then they go in the main menu to Show view->DITA references, then the keyboard focus is received inside the view. What does the narrator say when the focus is inside? If you are using TAB to move the focus, where does the focus go? Do they understand that they need to press the incoming/outgoing buttons in order to change the tree structure presented below? In the tree what does the narrator tell them for each reference? Do they know that they can press ENTER to open the reference.
Code review 4:
Code review part 5, am comis niste propuneri de simplificare a IncomingReference, sa pastram clar separate file name-ul si informatia aditionala de linie si coloana. Am mai eliminat niste cod pe care nu l-am inteles din IncomingReferencesPanel, nu stiu daca era necesar sau nu. Daca era necesar putem sa-l readaugam.
Code review part 6, metoda "isDisplayable" dintr-un Swing component nu inseamna ca se vede pe ecran. De exemplu se poate testa cu:
public static void main(String[] args) {
JFrame fr = new JFrame();
JLabel jl = new JLabel();
jl.setVisible(false);
fr.getContentPane().add(jl);
fr.setVisible(true);
System.err.println(jl.isDisplayable());
}
Trebuie testat cu "java.awt.Component.isShowing()", noi asta folosim cand vrem sa vedem daca o componenta chiar se vede sau nu pe ecran. Dar ce se intampla daca side view-ul nu este afisat, deschidem un topic DITA si apoi afisam side view-ul? Ar trebui atunci sa facem refresh-ul.
@mmmmmcr I think the issue can be closed.
com.oxygenxml.ditareferences.sideView=>Rename package to be all lower case com.oxygenxml.ditareferences.treeReferences => Rename package to be all lower case
SideViewComponent:
OnGoingReferencesTree => IncomingReferencesTree ReferencesTree => OutgoingReferencesTree outcomingReferences => outgoingReferences "References" => Needs to be translated refreshButton => Set a tooltip Why does the SideViewComponent add a component listener to the outgoingReferences panel?
OnGoingReferencesTree: OnGoingReferencesTree=> This is not really a JTree extension, maybe call it "OutgoingReferencesPanel".Also why not put the JScrollPane with the JTree inside this panel instead of adding it in the other place? OnGoingReferencesTree.oldEditorUrl=>currentEditorURL RO_SYNC_ECSS_DITA_DITA_ACCESS => DITA_ACCESS_CLASS_NAME com.oxygenxml.ditareferences.treeReferences.OnGoingReferencesTree.refresh(WSEditor) => All methods working with the JTree need to be called on the AWT thread com.oxygenxml.ditareferences.treeReferences.OnGoingReferencesTree.searchOngoingRef(URL) => We need to preserve the "graph" object as a field in the class for future use, so that we do not create it every time we ask for references. The "graph" field can be cleared when "Refresh" is pressed. referenceTree.setCellRenderer(new OnGoingReferencesTreeCellRenderer()); => The cell renderer should be set only once on the tree, there is no need to set it whenever the tree model changes. e1.printStackTrace(); => Add some logging system "Loading..." => Translate selectRange method=> the "getStartEndOffsets" API can be used also for the Text editing mode, because there is a common interface "ro.sync.exml.workspace.api.editor.page.WSTextBasedEditorPage" implemented by both the Text and Author pages. So maybe you can use that interface.