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

Magitek Clicker [Testing] #5134

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Magitek Clicker [Testing] #5134

wants to merge 2 commits into from

Conversation

ereti
Copy link

@ereti ereti commented Nov 30, 2024

it's a plugin for playing clicker sfx in response to chat messages.

it's pretty small but i figured it was worth submitting given that i am sure others will get use out of it

@bleatbot bleatbot enabled auto-merge (squash) November 30, 2024 01:06
@bleatbot
Copy link
Collaborator

bleatbot commented Nov 30, 2024

Outdated attempt

This is the first time that you have submitted a plugin here. Before the bot will build your plugin within the 'Build PR' check, someone from the approval team will need to enable builds for you.

Once this is enabled, the bot will automatically build the PR. Future iterations will not require an approval for building the PR, only merging.

Please hold!

@reiichi001
Copy link
Contributor

bleatbot, approve

@bleatbot
Copy link
Collaborator

bleatbot commented Nov 30, 2024

Outdated attempt

Builds failed, please check action output.

Take care! Please test your plugins in-game before submitting them here to prevent crashes and instability. We really appreciate it!

Name Commit Status
MagitekClicker [testing-live] 2081737 Build failed (Diff)
12 Needs (⚠️ 1 UNREVIEWED)
Type Name Version Reviewed by
NuGet NAudio.WinForms 2.2.1 ⚠️ NEW
NuGet NAudio 2.2.1 reiichi001
NuGet Microsoft.NETCore.Platforms 3.1.0 reiichi001
NuGet Microsoft.Win32.Registry 4.7.0 reiichi001
NuGet NAudio.Asio 2.2.1 reiichi001
NuGet NAudio.Core 2.2.1 reiichi001
NuGet NAudio.Midi 2.2.1 reiichi001
NuGet NAudio.Wasapi 2.2.1 reiichi001
NuGet NAudio.WinMM 2.2.1 reiichi001
NuGet System.Security.AccessControl 4.7.0 reiichi001
NuGet System.Security.Principal.Windows 4.7.0 reiichi001
1 hidden need (known safe NuGet packages).
Show log - Review

@bleatbot bleatbot added new plugin This is a new plugin. build failed This plugin failed to build. labels Nov 30, 2024
@ereti
Copy link
Author

ereti commented Nov 30, 2024

oh, i don't actually think i'm even using that, but it's a dependency through naudio?

is this something i need to resolve on my end, or do you just need to approve it as a dependency?

@reiichi001
Copy link
Contributor

Hi there! I've added this plugin to our review queue. There are a few things that I noticed which will need to be addressed:

  1. Plugin icon appears to not be in the correct format.
  2. Your repo URL is set to something else in https://github.com/ereti/MagitekClicker/blob/master/MagitekClicker/MagitekClicker.csproj#L8
  3. To use NAudio, you'll either need to implement it without windows forms being required (try using these nuget packages directly instead of full fat NAudio: NAudio.Core, NAudio.Vorbis, NAudio.Wasapi, NAudio.WinMM) or add <EnableWindowsTargeting>true</EnableWindowsTargeting> to your csproj to use them. Here's a recent example of when someone switched MKhayle/EldenRingDalamud@a065ed6#diff-39fd6c532af4b6c2bfa37c67bf64ad416cafb86b03e0d8d2a08c7dca3f231282R58

I also see you're using Dalamud.Plugin.Bootstrap.targets. This is being deprecated in favor of a custom .NET SDK solution. To future-proof things as we may eventually remove the targets file, you can swap these lines out.

<Project Sdk="Microsoft.NET.Sdk">
  <Import Project="Dalamud.Plugin.Bootstrap.targets" />

to

<Project Sdk="Dalamud.NET.Sdk/11.0.0">

The major version will always match Dalamud, but it will get minor updates every now and then.

@ereti
Copy link
Author

ereti commented Nov 30, 2024

got it - i'll fix 2 and 3 now.
i'm not sure what you mean about the icon though? it's a 128x128 png, and the only guidance i can see on it is that it should be a png between 512x512 and 64x64 nevermind, the png i included in this PR is for some reason totally broken compared to the one in my repo. will fix

auto-merge was automatically disabled November 30, 2024 02:31

Head branch was pushed to by a user without write access

@bleatbot
Copy link
Collaborator

Builds failed, please check action output.

Take care! Please test your plugins in-game before submitting them here to prevent crashes and instability. We really appreciate it!

Name Commit Status
MagitekClicker [testing-live] 3ae0cbb Build failed (Diff)
19 Needs (⚠️ 1 UNREVIEWED)
Type Name Version Reviewed by
NuGet NAudio.WinForms 2.2.1 ⚠️ NEW
NuGet NAudio 2.2.1 reiichi001
NuGet Microsoft.Build.Tasks.Git 1.1.1 goaaats
NuGet Microsoft.NETCore.Platforms 3.1.0 reiichi001
NuGet Microsoft.Win32.Registry 4.7.0 reiichi001
NuGet NAudio.Asio 2.2.1 reiichi001
NuGet NAudio.Core 2.2.1 reiichi001
NuGet NAudio.Midi 2.2.1 reiichi001
NuGet NAudio.Wasapi 2.2.1 reiichi001
NuGet NAudio.WinMM 2.2.1 reiichi001
NuGet System.Security.AccessControl 4.7.0 reiichi001
NuGet System.Security.Principal.Windows 4.7.0 reiichi001
7 hidden needs (known safe NuGet packages).
Show log - Review

@reiichi001
Copy link
Contributor

reiichi001 commented Nov 30, 2024

You'll want to remove NAudio 2.2.1 from your nuget packages when swapping to the more cross-platofrm friendly sets. It may have a few minor namespace changes.

@ereti
Copy link
Author

ereti commented Nov 30, 2024

to be clear, am i able to keep NAudio if adding <EnableWindowsTargeting>true</EnableWindowsTargeting>? that's what i tried to do above, and i don't understand the error i'm seeing in the build log from the github action.

i've tried switching to using the packages you recommended but i'm getting one long continuous headache trying to switch from AudioFileReader (which only exists in NAudio proper) to VorbisWaveReader

@reiichi001 reiichi001 added pending-code-review This plugin still needs code review. pending-rules-compliance This plugin still needs to be checked for rules compliance by the majority of the PAC. pending-testing This plugin still needs to be tested. labels Dec 5, 2024
@reiichi001 reiichi001 added completed-code-review Someone reviewed this. Check if the review wasn't invalidated already. completed-rules-compliance This plugin is compliant with all rules. and removed pending-code-review This plugin still needs code review. pending-rules-compliance This plugin still needs to be checked for rules compliance by the majority of the PAC. labels Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build failed This plugin failed to build. completed-code-review Someone reviewed this. Check if the review wasn't invalidated already. completed-rules-compliance This plugin is compliant with all rules. new plugin This is a new plugin. pending-testing This plugin still needs to be tested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants