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

New widget "Image Slideshow" #3406

Merged
merged 5 commits into from
Jul 1, 2018
Merged

Conversation

ar0ne
Copy link
Contributor

@ar0ne ar0ne commented Jun 22, 2018

Contains

This is new ui widget for slideshows images.

How to test

Currently, we don't have a bunch of associated images. But you could add for example "default" image.

e.g. GameDetailsScreen(line: 383).

    protected void setPreviewImage(TextureRegion texture) {
        UIImageSlideshow slider = find("preview", UIImageSlideshow.class);
        if (slider != null) {
            slider.addImage(texture);
            slider.addImage(Assets.getTexture(DEFAULT_PREVIEW_IMAGE_URI).get());
        }
    }

Outstanding before merging

Not sure but now it works automatically. Do we need to add buttons "next" and "prev" ? Or better to add new card and put it to backlog ?

@ar0ne ar0ne changed the title New simple widget "Image Slideshow" New widget "Image Slideshow" Jun 22, 2018
@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

Copy link

@syntaxi syntaxi left a comment

Choose a reason for hiding this comment

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

A few changes

public void update(float delta) {
if (isActive() && timestamp + speed * 1000 < getCurrentTimestamp()) {
timestamp = getCurrentTimestamp();
nextImage();
Copy link

Choose a reason for hiding this comment

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

You can use the delta here instead of making calls to Date.
Since delta is the time since last call you can do something like this:

public void update(float delta) {
    imageDisplayTime += delta;
    if (imageDisplayTime >= speed) {
        imageDisplayTime = 0f;
        /* Call your image switching here */
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea, thanks!

@Override
public void onDraw(Canvas canvas) {
if (currentImage.get() != null) {
canvas.drawTexture(currentImage.get(), Color.WHITE);
Copy link

Choose a reason for hiding this comment

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

Should this line be here? Won't it result in the image being drawn twice, once in the if below and once here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. thanks!


@Override
public void onDraw(Canvas canvas) {
if (currentImage.get() != null) {
Copy link

Choose a reason for hiding this comment

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

I'm making a suggestion that instead of manually drawing the image here, you make this widget use an UIImage.

You can simply call it's draw here and use a binding to automatically update the image in it when the image index here is altered.

You can also delegate to its getPreferredContentSize.

Copy link

Choose a reason for hiding this comment

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

Alternatively you can consider extending the UIImage which will also allow you to do similar and utilise it's default methods.

@syntaxi syntaxi added Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience Feature labels Jun 22, 2018
@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@Cervator
Copy link
Member

Tests out, nice work :-) I concur with Jelly's comments, happy to merge this after those are addressed.

On Slack we discussed the possibility of a timed cycle variant vs manual back/forward. That could be done in a separate PR or along with this one, up to you.

@GooeyHub
Copy link
Member

Uh oh, something went wrong with the build. Need to check on that

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@Cervator Cervator added this to the v2.1.0 milestone Jul 1, 2018
@Cervator Cervator merged commit 308ec22 into MovingBlocks:develop Jul 1, 2018
Cervator added a commit that referenced this pull request Jul 1, 2018
@ar0ne ar0ne deleted the image_slideshow branch July 6, 2018 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants