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

STARK: Implement the conversation log menu #1450

Merged
merged 12 commits into from Jun 24, 2018

Conversation

Projects
None yet
2 participants
@DouglasLiuGamer
Copy link
Collaborator

DouglasLiuGamer commented Jun 16, 2018

This PR is corresponding to implementing the conversation log menu in TLJ.

@DouglasLiuGamer DouglasLiuGamer requested a review from bgK Jun 16, 2018

@DouglasLiuGamer DouglasLiuGamer force-pushed the DouglasLiuGamer:branch-dialog branch from 7f1abd0 to 1fd4e49 Jun 16, 2018

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 17, 2018

@bgK The whole implementation is basically finished and it's ready for review.

I find it extremely difficult to make the layout of widgets exactly the same as the original, at least not in a reasonable way. I suspect that this is due to the difference of the graphics layers. In 93327fa I did my best to make the whole layout very similar. Differences should be unnoticeable unless you put it together with the original. If you think it is not satisfactory then I think we'll need to take a deep look at the disassembly.

Common::String name = StarkGlobal->getCharacterName(logLine.characterId);
name.toUppercase();

uint color = name == "APRIL" ? _textColorApril : _textColorNormal;

This comment has been minimized.

@bgK

bgK Jun 23, 2018

Member

This is how this test is done in other parts of the engine:

bool Speech::characterIsApril() const {

@bgK
Copy link
Member

bgK left a comment

This is looking very good! I've added a few minor comments that need to be addressed before this can be merged.

@@ -92,7 +92,7 @@ void StaticLocationScreen::onMouseMove(const Common::Point &pos) {
// The first widget is always the background. It is ignored below.

if (newHoveredWidget != _hoveredWidgetIndex) {
if (_hoveredWidgetIndex > 0) {
if (_hoveredWidgetIndex > 0 && _hoveredWidgetIndex < _widgets.size()) {

This comment has been minimized.

@bgK

bgK Jun 23, 2018

Member

Please fix the sign / unsigned comparison warning here.


++_nextTitleIndex;

if (pos.y > bottom) {

This comment has been minimized.

@bgK

bgK Jun 23, 2018

Member

Please fix the sign / unsigned comparison warning here.

class DialogLineText;

/**
* The coversation log menu

This comment has been minimized.

@bgK

bgK Jun 23, 2018

Member

Typo.

}

if (pos.y + height > bottom) {
break;

This comment has been minimized.

@bgK

bgK Jun 23, 2018

Member

When we break here the objects created above are never freed.

height = dialogLineText->getHeight();

if (pos.y + height + space > bottom) {
break;

This comment has been minimized.

@bgK

bgK Jun 23, 2018

Member

When we break here DialogLineText is not freed.

@bgK

This comment has been minimized.

Copy link
Member

bgK commented Jun 23, 2018

Differences should be unnoticeable unless you put it together with the original.

It looks close enough to me. I don't think anybody will mind if it's a few pixels of the original in this case.

@DouglasLiuGamer

This comment has been minimized.

Copy link
Collaborator Author

DouglasLiuGamer commented Jun 24, 2018

@bgK If you also think it's okay, we can merge.

@bgK bgK merged commit 32bbcd7 into residualvm:master Jun 24, 2018

1 check passed

default Build finished.
Details

@DouglasLiuGamer DouglasLiuGamer deleted the DouglasLiuGamer:branch-dialog branch Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.