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

Visually distinct UI elements for enabled/disabled #2288

Merged
merged 7 commits into from Jun 8, 2016

Conversation

Projects
None yet
4 participants
@tdgunes
Member

tdgunes commented Apr 9, 2016

Contains

It's going to fix #2277.

How to test

All the partial fixes can be/will be visible in showScreen migTestScreen.

Before merging

Previously:

  • UIButton - done!
  • UICheckbox - "kinda done - very faint difference in appearance? Could be more distinct?" (going to find a fix for that.)
  • UISpace - included for completeness, but pretty sure this one is N/A

PR currently includes:

  • UILabel - MISSING from the test screen, but otherwise supported and distinct (#2280) - just need to add it to the test screen for completeness
  • UISlider - no disable support (so naturally no look either)
  • UIDoubleSlider - no disable support (so naturally no look either)
  • UIText - can be disabled, but doesn't have a unique look yet
    • Capability of supplying initial text
    • A new dark texture
    • Has two different modes enabled and readOnly.
    • Disable cursor movement
  • UIDropdown - can be disabled, but doesn't have a unique look yet
  • UIDropdownScrollable - can be disabled, but doesn't have a unique look yet
  • UIList - no disable support, buttons have visual distinction so may just need the support
  • UIBox - N/A ? Not sure why we would disable one of these or have it show differently. Maybe if you're grouping elements and want to show the entire group as disabled (like a part of a tech tree you haven't gotten to yet) ?

@msteiger:

  • UIScrollbar - can be disabled, but not sure if it shows well enough on the test screen to see a difference in appearance?
  • Ability to enable/disable a widget to UIElement
  • Achieve an elegant and reliable way to enable/disable interaciton in a general manner
  • Run the checkstyle checks or integrate the plugin in your IDE so you can resolve the style issues
    screen shot 2016-04-21 at 03 22 17

@GooeyHub GooeyHub added the in progress label Apr 9, 2016

@Cervator Cervator added the UI label Apr 9, 2016

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Apr 11, 2016

Member

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/591/
Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Apr 11, 2016

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/591/
Hooray Jenkins reported success with all tests good!

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Apr 21, 2016

Member

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/608/
Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Apr 21, 2016

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/608/
Hooray Jenkins reported success with all tests good!

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Apr 21, 2016

Member

Uh oh - @tdgunes it looks like you've got some merges caught up in this PR sneaking in already-merged commits. While that usually isn't critical it clouds up the history and can make it a bit harder to review :-)

That usually happens if you use merges to keep a fork up to date with unique commits both in your fork and the upstream repo (the order of commits ends up differing). Using rebase can help avoid that (since you just put your changes in front of the latest head from upstream - no merges involved)

Depending on your comfort with Git there are easy, hard, or brute force ways to fix that. Let me know your preference sometime :-)

Member

Cervator commented Apr 21, 2016

Uh oh - @tdgunes it looks like you've got some merges caught up in this PR sneaking in already-merged commits. While that usually isn't critical it clouds up the history and can make it a bit harder to review :-)

That usually happens if you use merges to keep a fork up to date with unique commits both in your fork and the upstream repo (the order of commits ends up differing). Using rebase can help avoid that (since you just put your changes in front of the latest head from upstream - no merges involved)

Depending on your comfort with Git there are easy, hard, or brute force ways to fix that. Let me know your preference sometime :-)

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Apr 21, 2016

Member

@Cervator, sorry about that mess again.

What do think about this one?

  • UIScrollbar - can be disabled, but not sure if it shows well enough on the test screen to see a difference in appearance?

Personally, I am not sure either. :) Apart from that, I think it's ready for review.

Member

tdgunes commented Apr 21, 2016

@Cervator, sorry about that mess again.

What do think about this one?

  • UIScrollbar - can be disabled, but not sure if it shows well enough on the test screen to see a difference in appearance?

Personally, I am not sure either. :) Apart from that, I think it's ready for review.

@tdgunes tdgunes changed the title from [Work in Progress] Visually distinct UI elements for enabled/disabled to [Ready for review] Visually distinct UI elements for enabled/disabled Apr 21, 2016

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Apr 21, 2016

Member

Yay clean Git!

Maybe make the slider go gray if it is disabled? That would be visible including on the test screen even though you can't see the scroll bar itself much. Or maybe there's a way to force a fake larger body to attach the scrollbar to on the test page so you actually can scroll it? Maybe just add a bunch of "lorem ipsum" junk text somehow.

But yeah this is probably ready, just need time to test :-)

Member

Cervator commented Apr 21, 2016

Yay clean Git!

Maybe make the slider go gray if it is disabled? That would be visible including on the test screen even though you can't see the scroll bar itself much. Or maybe there's a way to force a fake larger body to attach the scrollbar to on the test page so you actually can scroll it? Maybe just add a bunch of "lorem ipsum" junk text somehow.

But yeah this is probably ready, just need time to test :-)

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Apr 21, 2016

Member

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/616/
Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Apr 21, 2016

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/616/
Hooray Jenkins reported success with all tests good!

@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger Apr 22, 2016

Member

Apologies for being late to the party. A few assorted notes:

  • I'd rather add the ability to enable/disable a widget to UIElement. The conditional cast to AbstractWidget isn't ideal imho.
  • There should be an easier way to disable interaction than checking every listener method. Afaik, you need to call Canvas.addInteractionRegion() to support interaction. Maybe you can wrap around that to achieve an elegant and reliable way to enable/disable interaciton in a general manner?
  • Please run the checkstyle checks or integrate the plugin in your IDE so you can resolve the style issues.

Everything else looks good to me.

Member

msteiger commented Apr 22, 2016

Apologies for being late to the party. A few assorted notes:

  • I'd rather add the ability to enable/disable a widget to UIElement. The conditional cast to AbstractWidget isn't ideal imho.
  • There should be an easier way to disable interaction than checking every listener method. Afaik, you need to call Canvas.addInteractionRegion() to support interaction. Maybe you can wrap around that to achieve an elegant and reliable way to enable/disable interaciton in a general manner?
  • Please run the checkstyle checks or integrate the plugin in your IDE so you can resolve the style issues.

Everything else looks good to me.

@tdgunes tdgunes changed the title from [Ready for review] Visually distinct UI elements for enabled/disabled to [Work in Progress] Visually distinct UI elements for enabled/disabled Apr 27, 2016

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator May 8, 2016

Member

Noticed another potential element to add while checking PR #2315 - UIImage - again maybe somewhat unorthodox for the lineup on the test screen, but should it be included just in case? Maybe we could have a little "misc" section for UI elements that aren't subject to disabling or resizing. Although I suppose UI images could actually be scaled?

Member

Cervator commented May 8, 2016

Noticed another potential element to add while checking PR #2315 - UIImage - again maybe somewhat unorthodox for the lineup on the test screen, but should it be included just in case? Maybe we could have a little "misc" section for UI elements that aren't subject to disabling or resizing. Although I suppose UI images could actually be scaled?

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 10, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 10, 2016

Hooray Jenkins reported success with all tests good!

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator May 11, 2016

Member

Huzzah, progress! :-)

As a reminder: Make sure you hook up and run Checkstyle. I see a few missing braces and a * import in there. Minor but good to keep consistent with the code conventions.

(if you haven't already that is - unsure if the pending work you mentioned on Slack went into that new commit or is still not submitted yet)

Member

Cervator commented May 11, 2016

Huzzah, progress! :-)

As a reminder: Make sure you hook up and run Checkstyle. I see a few missing braces and a * import in there. Minor but good to keep consistent with the code conventions.

(if you haven't already that is - unsure if the pending work you mentioned on Slack went into that new commit or is still not submitted yet)

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 22, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 22, 2016

Hooray Jenkins reported success with all tests good!

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator May 30, 2016

Member

Ran a round of testing on this, keeping #2329 in mind.

Merged it with latest develop first since the PR branch is a little behind. No conflicts or anything, just missing some stuff.

So long as I didn't use auto-completion (see #2329) everything looked good. I think the UIScrollBar is the only piece not covered from #2277.

I see 4 minor Checkstyle issues with star imports and a single missing bracket, so that's pretty close to done :-)

Should we still wait for the tweaking of widgets or what not? Referencing the checklist + @msteiger's comments. May be worth merging this and submitting a follow-up issue for later? Don't want it to get too dusty just sitting around :-)

Let me know @tdgunes! But only if you aren't in the middle of being swamped with more academics :D

Member

Cervator commented May 30, 2016

Ran a round of testing on this, keeping #2329 in mind.

Merged it with latest develop first since the PR branch is a little behind. No conflicts or anything, just missing some stuff.

So long as I didn't use auto-completion (see #2329) everything looked good. I think the UIScrollBar is the only piece not covered from #2277.

I see 4 minor Checkstyle issues with star imports and a single missing bracket, so that's pretty close to done :-)

Should we still wait for the tweaking of widgets or what not? Referencing the checklist + @msteiger's comments. May be worth merging this and submitting a follow-up issue for later? Don't want it to get too dusty just sitting around :-)

Let me know @tdgunes! But only if you aren't in the middle of being swamped with more academics :D

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Jun 7, 2016

Member

@msteiger: I'd rather add the ability to enable/disable a widget to UIElement. The conditional cast to AbstractWidget isn't ideal imho.

Thanks for fixing this @msteiger, I had a small local commit to resolve casting that you mentioned before by adding isEnabled() and setEnabled() abstract methods to UIWidget to remove casting completely.

Member

tdgunes commented Jun 7, 2016

@msteiger: I'd rather add the ability to enable/disable a widget to UIElement. The conditional cast to AbstractWidget isn't ideal imho.

Thanks for fixing this @msteiger, I had a small local commit to resolve casting that you mentioned before by adding isEnabled() and setEnabled() abstract methods to UIWidget to remove casting completely.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 7, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jun 7, 2016

Hooray Jenkins reported success with all tests good!

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 7, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jun 7, 2016

Hooray Jenkins reported success with all tests good!

@Cervator Cervator merged commit 510b23a into MovingBlocks:develop Jun 8, 2016

1 check passed

default Build finished. 502 tests run, 0 skipped, 0 failed.
Details

Cervator added a commit that referenced this pull request Jun 8, 2016

@Cervator Cervator added this to the Alpha 2 milestone Jun 8, 2016

@Cervator Cervator removed the in progress label Jun 8, 2016

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Jun 8, 2016

Member

Merged all the things closing both PRs and the issue. Huzzah! Thanks both :-)

Member

Cervator commented Jun 8, 2016

Merged all the things closing both PRs and the issue. Huzzah! Thanks both :-)

@tdgunes tdgunes changed the title from [Work in Progress] Visually distinct UI elements for enabled/disabled to Visually distinct UI elements for enabled/disabled Jun 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment