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

Fix small printing error in aiming window #39995

Merged
merged 1 commit into from
Apr 29, 2020
Merged

Fix small printing error in aiming window #39995

merged 1 commit into from
Apr 29, 2020

Conversation

olanti-p
Copy link
Contributor

@olanti-p olanti-p commented Apr 28, 2020

Summary

SUMMARY: None

Purpose of change

#37715 left 2 identical chunks of code related to range printing. Judging by what was printed before that PR, I think the author forgot to change the code to properly select between %d and %d/%d

Describe the solution

Format %d or %d/%d into a string, then display it

Testing

It compiles

@olanti-p olanti-p changed the title Fix condition in aiming window Fix a small printing error in aiming window Apr 28, 2020
@olanti-p olanti-p changed the title Fix a small printing error in aiming window Fix small printing error in aiming window Apr 28, 2020
@enaantd
Copy link
Contributor

enaantd commented Apr 28, 2020

I'm no C++ expert, but considering this is the exact same block two times, I don't quite understand the need for a label_range variable. Plus I think this refactoring changes the logic quite a bit?

Wouldn't this be more appropriate?

            if( dst != src ) {
                // Only draw those tiles which are on current z-level
                auto ret_this_zlevel = ret;
                ret_this_zlevel.erase( std::remove_if( ret_this_zlevel.begin(), ret_this_zlevel.end(),
                [&center]( const tripoint & pt ) {
                    return pt.z != center.z;
                } ), ret_this_zlevel.end() );
                // Only draw a highlighted trajectory if we can see the endpoint.
                // Provides feedback to the player, and avoids leaking information
                // about tiles they can't see.
                g->draw_line( dst, center, ret_this_zlevel );
            }

            // Print to target window
            if( ( panel_type == "compact" || panel_type == "labels-narrow" )
                && display_type != "numbers" ) {
                mvwprintw( w_target, point( 1, line_number++ ), _( "Range: %d/%d Elevation: %d" ),
                           rl_dist( src, dst ), range, relative_elevation );
                mvwprintw( w_target, point( 1, line_number++ ), _( "Targets: %d" ), t.size() );
            } else {
                mvwprintw( w_target, point( 1, line_number++ ), _( "Range: %d Elevation: %d "
                           "Targets: %d" ), range, relative_elevation, t.size() );
            }

@olanti-p
Copy link
Contributor Author

olanti-p commented Apr 28, 2020

Plus I think this refactoring changes the logic quite a bit?

Yes, it restores the behavior from before the mentioned PR, when Range: %d/%d was replaced with Range: %d only when current range was 0 (src == dst).

Wouldn't this be more appropriate?

It removes the duplication, yes, but doesn't restore the behavior.

@enaantd
Copy link
Contributor

enaantd commented Apr 29, 2020

My bad, I didn't read the description well enough. If this is a bugfix and not just simply refactoring, then my suggestion is indeed incorrect

Copy link
Contributor

@ifreund ifreund left a comment

Choose a reason for hiding this comment

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

Thanks!

@ifreund ifreund merged commit 8846a0a into CleverRaven:master Apr 29, 2020
@olanti-p olanti-p deleted the fix-range-display branch April 29, 2020 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants