Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replaced TableLayout with GridBagLayout, JXTaskPaneContainer #3927

Merged

Conversation

dominikl
Copy link
Member

@dominikl dominikl commented Jul 2, 2015

Replaces the usage of TableLayout with GridBagLayout in the left hand side "browser container"; this should fix 11403 and 12655.

Unfortunately the problems mentioned in the tickets are random and not reproducible, so for testing this PR make sure the left hand side panel in Insight still works as expected, i. e. switch between project, screen, administration, etc., change the size of the panel, make sure the scrollbar appears when there's not enough space, and so on.

@jburel jburel added the develop label Jul 2, 2015
}
}
src.setSpecial(true);
firePropertyChange(SELECTED_TASKPANE_PROPERTY, null, src);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure we needed that space 😃

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) I wonder how that slipped in. Unfortunately the whole formatting is a quite messy, couldn't manage to get my changes in matching the old format and if I reformat the whole file with the right format (i.e. spaces instead of tabs) one can't see the changes in the diff.

@gusferguson
Copy link

@dominikl

Tested using OMERO.insight-5.1.2-290-fdc6305-ice34-b549-mac_Java6 and OMERO.insight-5.1.2-290-fdc6305-ice34-b549-mac_Java7+ trout user-3 read-only-1.
Compared with OMERO.insight-5.1.2-233-27f65e6-ice34-b545-mac_Java6.

The right hand pane is not displaying the various tabs as expected. The un-expanded tabs are not anchored to the bottom of the window as expected - see screenshot - b549 on left and b545 on right.

There are also juddering and slow animations-transformations in the latest build when the tabs are expanded. This is compared with the completely smooth opening and closing previously.

The trees all appear to be behaving as expected, and the scroll bars are appropriate to the size of the pane and the contents.

main

@dominikl
Copy link
Member Author

dominikl commented Jul 3, 2015

Thanks @gusferguson ... Yes, I missed the point that the collapsed panes should be aligned at the bottom. The collapse/expand animations were already switched on previously but just didn't work with the TableLayout, now with the GridbagLayout they work. But I also find the animations slightly annoying. Shall I disable them (will match the previous behaviour)?

@gusferguson
Copy link

@dominikl - I think it would be better to disable them and revert to previous behaviour.

@dominikl dominikl force-pushed the remove_tablelayout_from_browsercontainer branch from f405cef to a58a9e9 Compare July 3, 2015 15:23
@gusferguson
Copy link

@dominikl

Tested using OMERO.insight-5.1.2-308-a751719-ice35-b550-mac_Java7+ user-3 read-only-1

Other tabs now pinned to the bottom - works as expected.
Transition when selecting other tabs smooth - behaves as expected.

Another issue noticed - with truncation of names in data tree. When a dataset with long names is opened (e.g. Project-user-3 > non-archivedv) the left hand pane horizontal scroll bar appears even though the names are accurately truncated to the width of the pane - see screenshot 1. If you then scroll horizontally, the pane appears to be expanded to the extent required to accommodate the long names even though they are actually truncated - see screenshot 2.

I tested the PR J-M did for the truncation a while back (can’t remember the number unfortunately) and we really struggled to get the behaviour correct with each solution tending to cause another issue, so the change to GridBagLayout may have negated or reverted one of those fixes.

This does not really affect the functionality of the data tree as if you just expanding the width of the panel the truncation expands correctly and you see the full image names, so I would be happy for you to look at that in another PR having considered and maybe talked to J-M about what was done before.

Therefore if you want to merge this PR and take up the scrollbars as a new issue, I am happy with that, but if you want to sort it out here I am happy to carry on testing.

main

main

@dominikl
Copy link
Member Author

dominikl commented Jul 6, 2015

I'll try to fix this issue here. The PR isn't urgent anyway, not likely to conflict with anything else, so no need for being merged immediately.

@dominikl
Copy link
Member Author

dominikl commented Jul 6, 2015

Just as a reference, found the 'name truncation' PR #3770

@dominikl dominikl force-pushed the remove_tablelayout_from_browsercontainer branch from a58a9e9 to 9c4d039 Compare July 6, 2015 13:19
@dominikl
Copy link
Member Author

dominikl commented Jul 6, 2015

I also noticed another issue: Previously for the name truncation just the width of the character 'A' was determined and then based on that calculated how many characters of the name have to be truncated. This often leads to wrong results (too much or too less cut off). Now the truncation happens iteratively until the name fits into the given space. This might have performance issues. Do we have a test dataset with hundreds of images with long names somewhere, to check if this change has significant performance implications? @gusferguson

@gusferguson
Copy link

@dominikl - no I don't think that we have a dataset - ask Simon though - I think he has scripts that make awkward datasets.

@dominikl
Copy link
Member Author

dominikl commented Jul 6, 2015

Example for dataset with 300 images with long names: read-only-1/user-3 "pngs"

@dominikl
Copy link
Member Author

dominikl commented Jul 6, 2015

Now I remember the reason for 7566591 (@jburel) again, it was the fix for the disappearing scrollbar issue, I just introduced it again. This will be more complicated to fix than I thought.

@gusferguson
Copy link

@dominikl - I expect so - it just about drove J-M nuts.

@dominikl
Copy link
Member Author

dominikl commented Jul 7, 2015

Hopefully fixed it without introducing other bugs...

@gusferguson
Copy link

@dominikl

Tested using OMERO.insight-5.1.2-426-49062df-ice34-b554-mac_Java6 trout user-3 read-only - pngs dataset.

Behaves as expected.
Horizontal scroll bar is still displaying when left hand pane is narrow and names truncated, but when the pane is expanded the scroll bar scales and disappears as appropriate when names no longer truncated. I think this is fine. Screenshot 1.
The truncation of the names is now being appropriately scaled to the width of the pane at all times. Screenshots 2 and 3.
No other odd behaviours seen and all other tabs and scroll bars behaving appropriately.

Good to merge!!

user-3 user-3 connected to trout open microscopy org

omero insight

user-3 user-3 connected to trout openmicroscopy org

@gusferguson
Copy link

@dominikl @joshmoore

Tested with Windows 8 and Ubuntu.
Behaviour consistent with Mac version - no obvious cross-platform issues.
Behaves as expected.
Even better to merge.

@joshmoore
Copy link
Member

Big 👍

joshmoore added a commit that referenced this pull request Jul 8, 2015
…rcontainer

Replaced TableLayout with GridBagLayout, JXTaskPaneContainer
@joshmoore joshmoore merged commit 26a6826 into ome:develop Jul 8, 2015
@jburel
Copy link
Member

jburel commented Jul 8, 2015

@dominikl: congrats

@dominikl
Copy link
Member Author

As @pwalczysko just discovered, I introduced a bug with this PR: If you now expand an empty dataset it shows a child node with the same name like the dataset (previously the child node just said "Empty"). Will fix this in a follow-up PR.

@sbesson sbesson added this to the 5.1.3 milestone Jul 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants