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

Allow exporting config and debug.txt with one click (on all platforms) #13605

Closed
wants to merge 9 commits into from

Conversation

grorp
Copy link
Member

@grorp grorp commented Jun 18, 2023

For fixing bugs reported by users, you often need their minetest.conf and debug.txt. However, it can be difficult for users to obtain these files. Desktop users have to find out the location of the Minetest user data directory on their platform. Android users can export debug.txt with a button in the app, but getting minetest.conf requires a computer with ADB installed if you have a recent Android version.

This PR adds a button for exporting so-called "debug info" to the about tab. This button exists on all platforms. On desktop, it copies to the clipboard, on Android, it opens a share pop-up.

This is how the new about tab looks on desktop:
new about tab on desktop

This is how the new about tab looks on Android:
new about tab on android

The so-called "debug info" consists of Minetest version, platform, active renderer, minetest.conf and debug.txt. Here's an example:

Minetest 5.8.0-dev-debug-1890e2c3-dirty
Platform: Android
Active renderer: OpenGL ES 3.2 V@0530.0 (GIT@8382da08aa, I139f36ad46, 1645197494) (Date:02/18/22)

minetest.conf
-------------
screen_w = 2412
font_size = 12
enable_3d_clouds = true
gui_scaling = 1.125
screen_h = 1080
maintab_LAST = about
menu_last_game = minetest

debug.txt
---------


-------------
  Separator
-------------

2023-06-18 19:36:57: ERROR[Main]: Game specified in default_game [minetest] is invalid.


-------------
  Separator
-------------

2023-06-18 19:45:54: ERROR[Main]: Game specified in default_game [minetest] is invalid.

Inspired by the discussion starting at https://irc.minetest.net/minetest-dev/2023-06-14#i_6091167. Builds on top of #12370.

To do

This PR is a Ready for Review.

How to test

Look at the about tab, verify that it looks as shown in the screenshots. Press the "Copy debug info" / "Share debug info" button and verify that the output is useful for bug reports.

Q&A

  • Where did the active renderer text go?

    I had to remove it to make space for the new button. However, I think that the active renderer isn't relevant for the average user anyway. If you really need this information, you can still get it via the "Copy debug info" button.

  • Why does the "minetest.net" button now use the full available width?

    Because the text on this button was cut off on my Android device.

@nerzhul
Copy link
Member

nerzhul commented Jun 23, 2023

it's a very cool idea but i prefer calling it "Export debug info"

@grorp
Copy link
Member Author

grorp commented Jun 23, 2023

it's a very cool idea but i prefer calling it "Export debug info"

On desktop, the debug info is copied to the clipboard silently. If the button was named "Export debug info" instead of "Copy debug info", the user wouldn't know that the debug info is copied to the clipboard and they would think that the button does nothing. Therefore, I think the current name is better.

@rubenwardy
Copy link
Member

rubenwardy commented Jun 23, 2023

it's a very cool idea but i prefer calling it "Export debug info"

It should be called "Share" or "Send" debug info on Android, as those are the specific terms on Android.

On desktop, "Copy" makes the most sense as that's what it's done - see above reply

@grorp
Copy link
Member Author

grorp commented Jun 23, 2023

It should be called "Share" or "Send" debug info on Android, as those are the specific terms on Android.

It's currently called "Share debug info" on Android, exactly as you're suggesting.

@rubenwardy
Copy link
Member

Please rebase, this is near the top of my review list

@rubenwardy rubenwardy added Rebase needed The PR needs to be rebased by its author. Feature ✨ PRs that add or enhance a feature labels Jul 30, 2023
@grorp
Copy link
Member Author

grorp commented Aug 21, 2023

Rebased.

@Zughy Zughy removed the Rebase needed The PR needs to be rebased by its author. label Aug 21, 2023
Copy link
Member

@rubenwardy rubenwardy left a comment

Choose a reason for hiding this comment

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

This should probably limit the lines of debug.txt to 30 lines

builtin/mainmenu/settings/dlg_settings.lua Show resolved Hide resolved
@rubenwardy
Copy link
Member

We should try to get as many things from the bug report template into here as possible, which includes:

  • Architecture
  • Build flags (This is already shown in the pause menu, so maybe see there).
    • This would include Lua vs LuaJit, and other dep info
  • CPU and GPU info

That last one will probably require porting code, so feel free to skip

@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Aug 30, 2023
@rubenwardy rubenwardy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Sep 3, 2023
@grorp
Copy link
Member Author

grorp commented Sep 3, 2023

This should probably limit the lines of debug.txt to 30 lines

Done, but limited to 64 instead of 30 lines.

Architecture

Build flags

Done.

CPU and GPU info
That last one will probably require porting code, so feel free to skip

I hereby skip that :)

@grorp
Copy link
Member Author

grorp commented Sep 3, 2023

string.tail is now a public API function. If that's not desired, what should I do instead? Leave it undocumented? Move it to a different file?

I think the "debug info" now contains enough information to be useful. Would it be possible to merge this as-is and, if someone wants to do that, add more information in future PRs?

@rubenwardy rubenwardy added the Concept approved Approved by a core dev: PRs welcomed! label Sep 17, 2023
@SmallJoker SmallJoker added the Rebase needed The PR needs to be rebased by its author. label Dec 26, 2023
@Zughy Zughy added Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR and removed Concept approved Approved by a core dev: PRs welcomed! labels Jan 21, 2024
@Zughy
Copy link
Member

Zughy commented Jan 21, 2024

@grorp rebase needed

@SmallJoker SmallJoker self-requested a review February 18, 2024 15:57
@@ -195,6 +195,27 @@ function string.split(str, delim, include_empty, max_splits, sep_is_pattern)
return items
end

--------------------------------------------------------------------------------
function string.tail(str, num_lines)
Copy link
Contributor

@appgurueu appgurueu Feb 18, 2024

Choose a reason for hiding this comment

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

Not very important, but if we're gonna expose this, I'd probably try to give it a clearer name ("tail" may also be in reference to characters rather than lines). I'd also probably implement this using a reverse numeric for: for index = #str, 1, -1 do local char = str:sub(index, index)...

@grorp
Copy link
Member Author

grorp commented Mar 3, 2024

There's a conceptual problem with this PR: Do we actually want minetest.conf/debug.txt at the beginning of every issue, together with the version information? While this would be useful for many bugs, it would be unnecessary noise for some issues, especially as you'd have to scroll down to reach the actual issue content. Having minetest.conf/debug.txt attached as downloadable files could be better.

If you agree, I can think of three options:

  1. Edit the issue template to instruct users to retrieve and upload minetest.conf/debug.txt using the "Open user data directory" button (on Android, include minetest.conf in the output of the existing "Share debug log" button)

  2. Automatically upload minetest.conf and debug.txt to some filesharing provider when the "Copy debug info" button is clicked and include links in the copied text (privacy?)

  3. Simply remove the minetest.conf/debug.txt-related stuff from this PR and limit this PR to copying version information, which is conceptually simple and especially useful because it includes stuff like the Irrlicht device

@grorp grorp added WIP The PR is still being worked on by its author and not ready yet. and removed Rebase needed The PR needs to be rebased by its author. labels Mar 3, 2024
@Zughy
Copy link
Member

Zughy commented Mar 3, 2024

Maybe option 3? Option 2 sounds like a headache privacy-wise, not sure about option 1. Regardless, I agree that minetest.conf and debug.txt aren't needed most of the times. Minimal tests are way more important

@grorp
Copy link
Member Author

grorp commented Mar 6, 2024

I'll go with option 3 and open a new PR with https://github.com/grorp/minetest/tree/about-tab-4 some day

@grorp grorp closed this Mar 6, 2024
@grorp grorp deleted the about-tab-2 branch March 6, 2024 17:09
@rubenwardy
Copy link
Member

I just want to note that the current share debug log button is the only good way to get the debug log on Android, without needing a computer. So that needs to remain

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ✨ PRs that add or enhance a feature @ Mainmenu Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR UI/UX WIP The PR is still being worked on by its author and not ready yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants