emilyploszaj / emi

A featureful and accessible item and recipe viewer
MIT License
221 stars 46 forks source link

fix #447 NPE #449

Closed shitlime closed 4 months ago

shitlime commented 4 months ago

Fixes #447

Turn on the HandRestock function of Tweakeroo, dig a block in survival mode, such as dirt, and then put it down immediately, it will cause a crash.

emilyploszaj commented 4 months ago

I'd prefer to actually resolve the underlying issue here with panels being null if possible, perhaps addWidgets should exit much earlier if EMI is in an error state because of tweakeroo?

shitlime commented 4 months ago

I'd prefer to actually resolve the underlying issue here with panels being null if possible, perhaps addWidgets should exit much earlier if EMI is in an error state because of tweakeroo?

You mean like this?

    public static void addWidgets(Screen screen) {
        SidebarPanel panel = panels.get(1);
        if (panel.space == null) {
            EmiLog.warn("panel.space is null, stop addWidgets.");
            return;
        }
        forceRecalculate();
        if (EmiConfig.centerSearchBar) {
            search.x = (screen.width - 160) / 2;
            search.y = screen.height - 21;
            search.setWidth(160);
        } else {
            search.x = panel.space.tx;
            search.y = screen.height - 21;
            search.setWidth(panel.space.tw * ENTRY_SIZE);
        }
        EmiPort.focus(search, false);

        emi.x = 2;
        emi.y = screen.height - 22;
        tree.x = 24;
        tree.y = screen.height - 22;
        updateSidebarButtons();
    }
shitlime commented 4 months ago

I'd prefer to actually resolve the underlying issue here with panels being null if possible, perhaps addWidgets should exit much earlier if EMI is in an error state because of tweakeroo?

You mean like this?

  public static void addWidgets(Screen screen) {
      SidebarPanel panel = panels.get(1);
      if (panel.space == null) {
          EmiLog.warn("panel.space is null, stop addWidgets.");
          return;
      }
      forceRecalculate();
      if (EmiConfig.centerSearchBar) {
          search.x = (screen.width - 160) / 2;
          search.y = screen.height - 21;
          search.setWidth(160);
      } else {
          search.x = panel.space.tx;
          search.y = screen.height - 21;
          search.setWidth(panel.space.tw * ENTRY_SIZE);
      }
      EmiPort.focus(search, false);

      emi.x = 2;
      emi.y = screen.height - 22;
      tree.x = 24;
      tree.y = screen.height - 22;
      updateSidebarButtons();
  }

After testing, this way of writing will cause the backpack search bar to be rendered to the upper left corner when it is first opened.

Then, due to my limited level, I did not find the specific reason why panels became null.

emilyploszaj commented 4 months ago

forceRecalculate(); should be setting up the panels, and only seems to bail if EmiScreenBase.getCurrent() is null, meaning there's no "real" screen open. This pattern is used in several places in EmiScreenManager and can be put at the start of this method instead. No need to warn, this is an expected case.

EmiScreenBase base = EmiScreenBase.getCurrent();
if (base == null) {
    return;
}
emilyploszaj commented 4 months ago

Just want to make sure this actually fixed the problem, and I am going to nitpick, the codestyle of the repo doesn't have single line ifs. If this works and you make the style change I'll happily merge and release for all versions in the next update. Thanks for reporting, debugging, and fixing this issue :)

shitlime commented 4 months ago

Just want to make sure this actually fixed the problem, and I am going to nitpick, the codestyle of the repo doesn't have single line ifs. If this works and you make the style change I'll happily merge and release for all versions in the next update. Thanks for reporting, debugging, and fixing this issue :)

This solves the problem nicely. And thank you very much for reminding.

emilyploszaj commented 4 months ago

Oh you were targetting the 1.20 branch, but all commits need to be merged into the main 1.20.4 branch and retargetting has caused a really big diff... can you remake the PR based on 1.20.4? I'm sorry for all the work I'm asking of you for such a simple change but I can't merge it without that :sweat_smile:

emilyploszaj commented 4 months ago

I didn't see this was resolved, sorry for the late merge.