Bug 417829

Comment 1 Alexandra Schladebeck 2013-09-26 04:47:35 EDT

After speaking to Achim, we'd like to see this fixed for the upcoming version 7.2.

Comment 2 Alexandra Schladebeck 2013-11-15 02:55:00 EST

If two test results are open, and neither of them has focus, then the order is undefined. One of the results should spring to the next error, but the user should place the focus in the desired view if there are more than one open.

Comment 3 Maissmaallsmyss Maulhs-Vvuillss 2013-12-03 06:33:39 EST

Created attachment 237947 [details]
patch for Bug 417829

Comment 4 Maissmaallsmyss Maulhs-Vvuillss 2013-12-03 06:34:10 EST

After discussion with Alex we decided to use implementation containing in this patch (attachment 237947 [details]) and to work with it for some time to see if we need to do any additional changes

Comment 5 Markus Tiede 2013-12-04 04:18:19 EST

After talking to AS adjusting the target milestone accordingly.

Comment 6 Markus Tiede 2013-12-04 04:47:45 EST

Please adjust the following within your patch:
 - there are multiple empty line changes within the patch: remove / undo them before creating the patch
 - the PartListenerToTrackTRParts is registered within the "JubulaWorkbenchWindowAdvisor" which only gets used within the standalone context: move the listener registration so that it's also going to work when running as a plugin e.g. in Eclipse for Testers
 - within the "PartListenerToTrackTRParts" class implementation / and at multiple other locations you're accessing a singleton instance multiple times - avoid that and only access it once
 - within the "PartListenerToTrackTRParts" class implementation you're using unnecessarily nested if-clauses - avoid that
 - the TestResultsPartsTracker singleton is responsible for maintaing the views / editors state: hence it should not have any methods like "disableGoToButtons" - this is something the expression / handler decides on his own when reacting on state changes
 - in the "partBroughtToTop" method within the "PartListenerToTrackTRParts" class do not catch NPEs
 - do not add // uncommented lines within your patch
 - keep in mind when accessing "PlatformUI.getWorkbench().getActiveWorkbench().getActivePage()" the "getActive...()" calls may return null
 - try to single-source the logic for tracking the visibility / hidden status of the view
 - why is it still necessary though you've implemented and active view tracker to <or></or> the activeWhen status of the handler with the activePartId: this information seems redundant and should be removed
 --> this also leads to expression definitions that are hard to understand / mixing states and behavior (e.g. variable "*.areGoToErrorButtonsDisabled", definition "*.distinct.activeWhen.GoToTestResultErrrorInNotFocusedView")
 --> this also affects the ActiveProjectSourceProvider (which is by the way also not the correct location to place this code as it's not necessarily related to a project being active)
 - do not include modified dashboard launch configurations within your patch

Comment 7 Maissmaallsmyss Maulhs-Vvuillss 2013-12-06 08:10:37 EST

Also it was decided to create a new SourceProvider instead of using ActiveProjectSourceProvider to avoid redundant calls of provider in cases that do not correspond actual changes of active project state.

Comment 8 Maissmaallsmyss Maulhs-Vvuillss 2013-12-06 08:19:59 EST

After discussion with Markus next steps are considered as the next step:

10) - why is it still necessary though you've implemented and active view tracker to <or></or> the activeWhen status of the handler with the activePartId: this information seems redundant and should be removed
	10.1 --> this also leads to expression definitions that are hard to understand / mixing states and behavior (e.g. variable "*.areGoToErrorButtonsDisabled", definition "*.distinct.activeWhen.GoToTestResultErrrorInNotFocusedView")
	10.2 --> this also affects the ActiveProjectSourceProvider (which is by the way also not the correct location to place this code as it's not necessarily related to a project being active)

11) think about reducing the amount of handler from 4 --> 2 (command parameter)

It will be decided if these steps will be implemented after re-creating and posting the patch with other adjustments.

Comment 9 Maissmaallsmyss Maulhs-Vvuillss 2014-01-13 12:03:53 EST

Unfixed bug #398433 causes the NPEs in in the "partBroughtToTop" method within the "TestResultPartsTracker" class. It was decided to fire a ticket for NPEs after this patch´s approval, which will depend on the bug 398433. Then it will be possible to decide - if it is better to wait until the origin bug will be fixed or to fix the ticket independently.

Location of the issue with NPEs:
package org.eclipse.jubula.client.ui.rcp.controllers;
public class TestResultPartsTracker
...
/** {@inheritDoc} */
        public void partBroughtToTop(IWorkbenchPart part) {
            if (part instanceof TestResultTreeView) {
                setTRTreeViewHiddenStateTo(false);
            }
            if (!((part instanceof TestResultTreeView)
                  || (part instanceof TestResultViewer))) {
                if (part instanceof IViewPart) {
                    IWorkbenchPage actPage = Plugin.getActivePage();
                    if (actPage != null) {
                        // TODO: resolve issue with NPEs
                        IViewPart[] stackedViews =
                                actPage.getViewStack((ViewPart)part);
                        for (IViewPart iViewPart : stackedViews) {
                            if (iViewPart instanceof TestResultTreeView) {
                                setTRTreeViewHiddenStateTo(true);
                            }
                        }
                    }
                }
            }
        }

Comment 10 Maissmaallsmyss Maulhs-Vvuillss 2014-01-13 12:09:48 EST

Created attachment 238932 [details]
patch for Bug 417829

Updated patch, which include discussed changes (it was decided to think of some other changes, like to reduce the amount of handlers, and to change <or></or> in the activeWhen status of the handler with the activePartId, after the main changes will be approved).

Comment 11 Markus Tiede 2014-01-14 10:41:29 EST

Please adjust the following:
 - the patch is not applicable for me if I do no explicitly use the option "--ignore-whitespace" for git apply. (which leads to a pretty big diff - though nothing is acutally changed - for the JubulaWorkbenchAdvisor)
 - as far as I can see the "EditInteractionMonitor" is though implementing the IClientStatusListener interface nowhere registered as such
 - replace / remove the usage of "Import-Package: " in the MANIFEST of org.eclipse.jubula.client.ui.rcp - use require bundle instead (an even before that: explain to me what this newly introduced dependency is required for)
 - please explain why there is a newly introduced variable which begins with "may..."
 - provide i18n keys / values for Strings used (e.g. in the register workbench startup listener job)
 - do not include uncommented code in the patch (as e.g. in the GuiEventDispatcher)
 - avoid NPEs in "getTestResultPartToExecuteGoToOn()" - as there might be no active...() + why do you check the length attribute of the view / editor references?

And please as soon as re-attaching the next patch mark the old one as "obsolete".

Comment 12 Maissmaallsmyss Maulhs-Vvuillss 2014-01-27 08:02:42 EST

Created attachment 239343 [details]
patch for Bug 417829

DONE
 - as far as I can see the "EditInteractionMonitor" is though implementing the IClientStatusListener interface nowhere registered as such
the interface implementation was deleted as unneeded
 - replace / remove the usage of "Import-Package: " in the MANIFEST of org.eclipse.jubula.client.ui.rcp
import was removed
- do not include uncommented code in the patch (as e.g. in the GuiEventDispatcher)
code in Gui... was deleted
- provide i18n keys / values for Strings used (e.g. in the register workbench startup listener job)
done in WorkbenchStartupListenerIsRegistring, RegisterWorkbenchStartupListener messages in org.eclipse.jubula.client.ui.rcp/src/org/eclipse/jubula/client/ui/rcp/i18n/messages.properties
- why do you check the length attribute of the view / editor references (getTestResultPartToExecuteGoToOn(), AbstractGoToTestResultErrorHandler)?
check was removed
- avoid NPEs in "getTestResultPartToExecuteGoToOn()" - as there might be no active...(AbstractGoToTestResultErrorHandler)
NPEs are checked and logging added on check failure
- please explain why there is a newly introduced variable which begins with "may..."
explained in comment before a definition of a variable "org.eclipse.jubula.client.ui.variable.mayTestResultViewerBeHidden" in org.eclipse.jubula.client.ui.rcp/plugin.xml
and in a description of m_isTRTreeViewHidden member of the TestResultPartsTracker class
<!-- currently it is impossible easy to check if the part (view/editor) 
        was hidden by other editor, because no list of stacked editors exist (only of staked views);
        state of TestResultViewer is regarded as "may be hidden" when (this behaviour is defined
        in methods of PartListenerToTrackTRParts and other methods of TestResultPartsTracker):
            - editor other then TestResultViewer was activated ;
            - on Perspective change;
            - when TestResultViewer was closed;
-->

Comment 13 Alexandra Schladebeck 2014-02-03 10:11:51 EST

We've discussed that there would still be about a day of effort to prepare, review, alter and then commit the current patch. We're not going to do this at the moment, since the problem is not that bad. Marking as wontfix.

Comment 14 Oliver Goetz 2014-02-04 02:53:32 EST

Closed due to comment 13