MuseScore はじめてのPR

ひとつの壁を乗り越えたのでプロセスの記録＋今後のための備忘録。

Issue を選ぶ

まず解決できそうな問題を探す。まずGitHubのissuesの見方から調べる。

issue の形式的判断（筆者が拾っても良いタスクか）

OSSプロジェクトは他の開発者との共同作業になる。MuseScoreプロジェクトでは数百人単位の参加者がいる。そのため、チームワークを乱さないための情報収集からはじめる。OSSプロジェクトのチームの構造、権限の分配を意識しながら、筆者が着手して良い Issue であるかを見極める。

  • Open / Closed はそのissueが解決したかどうかを表す。
    • Open（未解決）だけど誰かが作業中、PRなし、というのは普通の状態。
  • Status 確認
    • Tentatively in progress : 仮に進行中。誰かが既に担当or準備中。
    • In Progress : 進行中
    • Available : 拾えるタスク
  • Linked PR がある issue は基本的に拾わない。
    • 誰かがすでに着手している issue なので。
    • ただし、PRが数ヶ月放置されていたり、誰も作業していないケースもあるので、その場合はメンテナーにコメントすると拾えるケースもある。
    • issue は Open / PR が Close された→拾える。しかも失敗の前例があり学びになる。

issue の実質的判断（筆者が拾う意義のあるタスクか）

単に「OSSでPR申請をだした」実績を作りたいだけなら軽いタスクを拾えば良い。しかし、プロダクトの中身の深い理解につなげたいのであれば、学習効果の高い Issue を掘り当てていきたい。また初心者がひとりで解決可能なレベルのタスクであるかも吟味する必要がある。

  • issue の性質
    • Bug : バグ修正
    • Feature : 機能改善、新機能追加
    • Task : 他の必要な開発作業（リファクタリング、内部処理書き換えなど）
    • バグ修正タスクは周りを壊しそうで怖いし、むしろ機能追加タスクがやりやすいかも。
  • Labels で issue の特徴がわかる。
    • good first issue がはじめての人にはおすすめ。
    • Label を貼る立場視点だと、小規模ですぐに安全に直せるタスクと、システム全体の調整が必要になるタスクを判断できるかが腕の見せ所。
  • Issue の内容的な深さがあるか（学習素材になるか）
    • 「UI仕様の単純変更」タスクでは、データ構造に触れたり複数レイヤーのファイルを読み込まなくても済む。MuseScoreのコアを理解する訓練にはつながらない。
    • 事例：Don’t add “Subtitle” placeholder text to new scores #24459
  • 解決可能な Issue か
    • 3年前に出された Issue で未解決、Available → 技術的に手を付けにくい、地雷系のリスクがある。初心者向けではない。
    • Issues には「技術的な修正で済むタスク」と、「仕様の方向性を議論すべきタスク」が混在しており、後者に手を付けてPR申請してもすぐには承認されない。後者の議論はコアメンバーを巻き込んで合意形成しないといけないため。
    • 事例：Header and Footer Entry dialog does not open on double click #13753
  • シンプルなUI変更 Issue が大きな話に広がった例
    • 見た目の改善→Layoutパネルの設計の問題→アクセシビリティ問題浮上、と論点が広がる。さらに、将来的な設計との整合性にも考慮すると、コミュニティ向け Issue からコアチーム対応の Issue に格上げすべき状況となった。
    • 極小規模な修正でもシステム全体に波紋を広げる場合もあり、システム全体が整合するように意図した修正を加えるのは簡単ではない。システムは生き物のように動く。
    • 事例：Linked staves representation in Layout panel: Use icon instead of [LINK] #26532

筆者が今回挑戦した Issue 概要

  • 「フレットボード・ダイアグラムの指番号欄に、数字(1-5)だけでなく T/t, P/p も入力可能にする」
    • MuseScore4 の仕様が現実の音楽文化の記法に追いついていない背景があるので、音楽的な意味のある改善。
    • UI 的には受け入れ可能文字列の範囲を変更し、描画ロジックも修正し、データ構造にも変更を加えるタスクになる。
  • Fretboard diagram properties: Accept T, t, P, and p as valid fingerings #23755

PR 出すまでの流れ（外部とのやりとり）

PRでは、コミュニケーションと責任範囲を的確に読み取る。

  • 作業開始の宣言を Issue コメントに出す
    • すでに作業中・相談中の人がいるかもしれない。礼儀・安全策として一言宣言。
    • “I’d like to work on this issue. If no one is currently working, may I take it?”
  • PR 作成
    • Issue 番号明記 (Fixes #xxxxx)
    • 何を変えたのか、なぜ変えたのか
    • スクショ・動画があれば添付
  • レビュー対応

Issue に取り組む

調査・見通しをつける

  • ファイル構造から当たりを付ける。
    • UI (QML) の制御
    • ドメインロジック、内部のデータ構造
    • レンダリング（描画）の処理
    • ファイル構造は今年3月頃にChatGPTと相談しながら深掘りしてきたので、そのときの感覚を思い出しながら調査する。
  • UI制御
    • inspector/qml/notation/fretdiagrams
    • 入力欄 (TextField)、文字入力制限 (Validator)、UI のバインディング（内部への値反映）を調べる。
    • 次の記述があれば、数字以外入力できない仕様なので変える必要性が出る。
      • validator: IntValidator { … }
    • 上記フォルダに8つのファイル(.cpp/.h/.qml)があり、その中から書き換えるべきファイルをピックアップする。
  • データ構造
    • engraving/dom/fret.cpp, fret.h, fretDiagram.cpp
    • フィンガリングデータをどの型で扱っているか？　char / int / enum ?
    • int型で保存しているなら、T/Pの入力対応には内部データから型を変える必要があるか？　それとも内部データはint型のままで、表示だけ文字列に対応させるか？
    • →データ的にはint型だった。ひとつの運指のデータを、vector<int> でひとまとめに保管している。（例：6弦なら、{2,3,4,0,0,0} ）
  • レンダリング
    • 現在、数字前提のレイアウトなので、T,Pを受け入れたときに描画が崩れないか要確認
  • 改善の方向性
    • T,t,P,p を、4種として拡張するのでなく、T,Pの2種として拡張する。
    • t,pはそれぞれ内部的にはT,Pと同じと見なす。UIのみ受付可能とする。

何を、どこに、どんなふうに記述するか

  • 内部処理→インターフェース→UI　の順で手を加えると良さそう。
  • 入力・内部処理では扱うデータの制約強め、出力（描画）では緩くするとバグを出しにくくして拡張性を確保できる。
    • 描画層で数字だけ描けます、といったコードだと、後々面倒。「渡された文字列を描く」にしておけば、後の拡張で内部処理のみいじれば良い。描画層で意味を知らないほうがいい。
  • 関数の順番
    • 宣言→関数定義の順であればコンパイラ的にはOK
    • ただし、人間が読む観点では、ヘッダの宣言順 ≒ cpp の定義順に揃えたい
    • 1行で済む簡素な関数は、.h ファイルで宣言と同時に定義する場合もある
  • 指番号の内部表現をどこに定義すればよいか
    • FretDiagram クラスのなかに、 enum FingeringValue として定義
    • 外から参照されるべきなので、public 内に配置
    • .cpp でなく .h に定義する
      • cppに書いた型はそのファイルからしか参照できない。.hに書かないと他の.cppから参照できない。
    • enum を定義する目的は、魔法の数字 (magic number) を根絶して、修正点を1カ所に集約すること。意味の変更を将来するとなると、全ファイルで修正しないといけなくなる。
  • 文字列・数値の変換ヘルパーはDOM層に書く。
    • データの意味を理解するのはDOM（内部処理）層に限定。レイアウト層が知るべき話ではない。layout層では、意味をしらないまま与えられた文字列の配置だけを計算する。
    • 今回の変換は、他の場面でも使う可能性がある。他のレイヤーに記述すると、その関数は描画専用にしか使われない。後々、別のレイヤーで再実装すると保守困難に。
  • getter, setter
    • C++ ではメンバ変数をpublicにさらさないで、関数を通してしか触れないようにする。
    • getter: クラスの中の値を読み取るための関数
    • setter: クラスの中の値を書き換えるための関数
    • UI 層から DOM のデータを直接触るようなコードを書くとカオスになる。バグを特定できなくなる。ダメな例：
      • score->elements[123].m_fingering[4] = 6; //123番要素の5弦の運指を "T"
    • setter で値の妥当性検証はしない設計で、整形済みの値を受け取るだけが望ましいらしい。そこで、T-6, P-7 の変換は別関数を用意。
  • DOM, Inspector Model
    • DOM: スコアの本体データそのもので、音楽的な意味を持っている
    • Inspector Model: UI (QML) と DOM の間を橋渡しする
    • QML の TextField から拾った生の文字列を読むのは Inspector の仕事で、DOM のレイヤーで処理されるのは不適切。
  • 文字列の型
    • Qtで扱う文字列型：QString
    • C++標準の文字列型：std::string, string
    • MuseScore独自の文字列型：muse::String, String
    • Charも同様に、C++標準の char とは別に、MuseScore独自の Char 型が存在する。
  • 関数の命名
    • 1文字を受け取る関数を定義するとき、名称は xxxFromString より、xxxFromChar
    • 逆に文字列に変換する関数を定義するとき、xxxToString が自然。1文字でも複数文字でも String で渡せばレイアウト側で表示処理がされる。
  • フィンガリングのレイヤー間のデータの型・中身の流れ
場所データ型例
インスペクタ UI (QML)QString“3”, “T”, “p”
インスペクタ C++
setFingering		QString → [Char → int] → QString → QString

*[ ] の変換でfingeringFromCharを呼び出し		6弦楽器の運指表示で、4弦をTにする場合
“T” → [“T” → 6] → “6” →
“0,0,0,6,0,0”
.mscx に保存されるデータQString“0,0,0,6,0,0”
DOM
fingeringFromChar		Char → int“T” → 6
DOM
fingeringToString		int → String6 → “T”
インスペクタ C++
fingerings → displayFingerings		QString → QStringList → [int → String] → QString

* [ ] の変換でfingeringToStringを呼び出し		“0,0,0,6,0,0” → {“0″,”0″,”0″,”6″,”0″,”0”} → [ 6 → “T”] → “T”
楽譜描画
layoutFretDiagram		int → String

*QString出発ではないの？		6 → “T”

C++ の知識、書き方

  • static : クラスそのものに属する関数（インスタンスでなく）
    • そのクラスの共通ルールを提供する。
    • static 関数には this をもたない。
  • 値渡し、参照渡し(&)、ポインタ渡し(*)
    • Char 型は内部的に整数1つを持つだけの軽量な型→値渡しでOK
    • 重いオブジェクト (String, std::vector など) はconst String& s のように参照渡しする。
    • ポインタ渡し：nullptr が来る可能性がある、生存期間を気にすべきとき、所有権問題が起きるときに使うらしい。

Qt QML の知識

  • QMLの比較演算子
    • JavaScript と同じルールで評価されるらしい。
    • == だとゆるい比較（型変換を認める）、=== は厳密な比較（型変換を認めない）
  • Qt QML の正規表現を validator で使う
  • QML と C++ の連携
    • Q_PROPERTY: C++のプロパティをQMLに公開する仕組み
      Q_PROPERTY(型 プロパティ名 READ getterメソッド WRITE setterメソッド NOTIFY シグナル名 オプション)
    • Q_INVOKABLE : QMLから呼び出すために明確に公開された C++ メソッド

変更点まとめ

  • DOM (fret.h / fret.cpp)
    • 運指の内部表現定義：enum class FingeringValue : int
    • 文字列→内部値変換：int FretDiagram::fingeringFromChar(Char c)
    • 内部値→文字列変換：String FretDiagram::fingeringToString(int v)
  • 描画 (tlayout.cpp)
    • 出力すべき文字列 String fingerS を次のように書き換える。
      String::number(finger)　→　FretDiagram::fingeringToString(finger)
  • インスペクタ (fretdiagramsettingsmodel.cpp / .h)
    • 運指のセッター：Q_INVOCABLE void setFingering(int string, int finger)　で、第2引数を const QString& fingerText に型変更
    • UIから受け取るのが int型 から QString型 に変わるのにあわせて、受け取った文字列をfingeringFromChar(Char c)で変換する。
  • UI (FretDiagramSettings.qml)
    • TextField の入力制限を、数値だけ(0-5)から、T,t,P,p も許容するように正規表現で設定
    • UI→インスペクタには、int型でなくQString型で渡す仕様に変更
  • インスペクタプレビュー (fretcanvas.cpp)
    • tlayout.cpp と同様の書き換え。
    • ここの書き換えを忘れて、はじめは次のような表示の不一致が発生した。

PR を出してみる

  • Issue スレッドでやること
    • 自分がタスクを引き受けたい宣言をして、メンテナーの承認を得る
    • ここで技術的説明、動作確認のスクショなどは不要
  • PR でやること
    • 何を解決したか (Fixes: #xxxxx) 、および技術的な変更点を説明する。
    • must: 何をしたかを記述、option: なぜそうしたかの深い議論を記述
    • 本件特有の事情
      • DOM に 6,7 を内部コードとして追加した点はPRに特記した。
      • 理由：バージョン間の互換性に影響する、データ仕様に手を加えたことになるため。
    • 次のスクショのとおり、旧バージョンでT,P対応の楽譜データを読み込むと、以下の表示となる。後方互換性についてどうするかを懸念点として記述した。
  • PR を作る
    • GitHub サイトで自分の fork を開く。
    • Compare & pull request をクリック
    • タイトル・本文を書く
    • 下記8項目のチェックリストが出てくるので、内容を確認してチェックを入れる。
    • Allow edits and access to secrets by maintainers
      • コアメンバーが筆者のPR ブランチに直接書き込みできるようにする設定
      • ONにしておいて損はない。
    • 記述内容は次のとおり。（英訳はChatGPT 任せ。ありがたい）
  • PR フォームに出てくる8項目のチェックリストの日本語訳
[ ] I signed the CLA

[ ] The title of the PR describes the problem it addresses
[ ] Each commit’s message describes its purpose and effects, and references the issue it resolves
[ ] If changes are extensive, there is a sequence of easily reviewable commits
[ ] The code in the PR follows the coding rules

[ ] There are no unnecessary changes
[ ] The code compiles and runs on my machine, preferably after each commit individually
[ ] I created a unit test or vtest to verify the changes I made (if applicable)		●私は CLA（Contributor License Agreement）に署名しました
●PR のタイトルは、解決しようとしている問題を適切に説明しています
●各コミットメッセージは、その目的と影響を説明し、関連する issue を参照しています

●変更が大規模な場合、レビューしやすいよう複数のコミットに分けています
●この PR のコードは、MuseScore のコーディング規約に従っています
●不要な変更は含まれていません
●コードは自分の環境でコンパイル・実行できています（可能なら各コミットごとに）

●（必要な場合）今回の変更を検証するための unit test または vtest を作成しました
  • GitHub で自動処理される diff (差分)表示について
    • 作業者の意図とはズレた差分表示になったりするが、diff がきれいに見えるようにコードの書きかたを変えることまではふつうしないらしい。
    • ただし、ロジック変更・単なる整形を同じコミットに混ぜない等の配慮はできる。
    • 下の例で、setFingering を旧193行目で消したみたいになっているが、実際は新212行目に移しただけ、なのだが表示のうえでは紛らわしくなってしまった。
  • PR出してから、CIチェックが走る
    • CI : Continuous Integration　（継続的インテグレーション）
      • PRが出た瞬間に、機械にレビューさせる仕組み。
      • よくあるチェックの種類は、ビルドチェック、テスト実行（ユニットテスト）、コードスタイル、静的解析、ビジュアルテスト・回帰テスト。
    • GitHub公式解説：https://docs.github.com/ja/actions/get-started/continuous-integration
    • 初回、5つのチェックでコケる。うち3つはシステム側のエラー、残る2つはこちらのコードに問題があったらしい。
    • Without Qt : Qt をリンクしない最小構成のビルド＋テスト。Qt依存のコードが紛れていたらしい。
    • Codestyle : スタイルチェック。if (...) の間のスペースがなかったりするとエラーが出る。
    • skipped checks : 依存タスクが失敗していたり、そもそも不要な項目の場合に飛ばされる。
  • CI エラー対応①：Codestyle
    • 解決策：uncrustify で自動整形する。
    • 手動でダウンロード：https://github.com/uncrustify/uncrustify/releases
    • PATH を通す：C:\Program Files\uncrustify　など
    • PowerShell で確認：uncrustify --version
    • 次のコマンドを Gitbash で動かして整形処理
    • 作業手順としては、fetch/rebase upstream→uncrustify→ビルド確認→commit→push の順序がよい。
// 直近の1コミットで変更されたファイル一覧
git show --name-only

// diffにすると、master→今のブランチHEAD までの変更ファイルすべてを出す
git diff --name-only upstream/master...HEAD

// uncrustify
git diff --name-only upstream/master...HEAD \
  | grep -E '\.(cpp|h)$' \
  | xargs tools/codestyle/uncrustify_run_file.sh
  • CI エラー対応②：Without Qt
    • エラーログをChatGPTに解析させると、文字列型の変換に問題があったらしい。
    • String("T") → String(u"T") に書き換えて解決
    • 背景：muse::String 型は内部で UTF-16 (char16_t) と独自 Char 型を使っているが、Qt なしビルドだとこのへんの型変換を厳密に意識しないとコンパイルが通らないらしい。
  • reviewer について
    • PR を読んでコメントできる立場の人。複数のポジションがあるらしい。
    • 一般 contributor : 誰でもできる。コメントや改善提案は自由にできるが、承認・マージはできない。
    • Trusted reviewer : コアメンバーに準ずる扱い。承認はできる場合あり、マージはできないことが多い。
    • Core maintainer : コア開発チーム・スタッフ。マージできるのはこの層だけ。

VSCode, 開発環境整備

Qt のバージョンが master と整合しない問題

  • 8ヶ月前の環境では、Qt 6.5.3 導入。
  • いまのmasterの要求は、6.8以上必須に。
    • C:\Qt\MaintenanceTool.exe を起動して、「コンポーネントの追加・削除」で Qt6.8.x を追加インストール。使用コンパイラによって複数選択肢があり、msvc2022_64 選択。
Could not find a configuration file for package "Qt6" that is compatible
with requested version "6.8".

C:/Qt/6.5.3/msvc2019_64/lib/cmake/Qt6/Qt6Config.cmake, version: 6.5.3
  • コンポーネントも複数必要で、6.8.3 に揃えたものを合わせてインストール。追加したコンポーネントは下記4つ。
    • Qt 5 Compatibility module
    • Qt Network Authorization
    • Qt Shader Tools
    • Qt State Machines
  • 開発環境、依存する外部ツールの保守という土台を身体で経験する時間…。開発の現場は、コードの修正だけでなく、地ならしまで求められる。

Qt component “GuiPrivate” が見つからない問題

  • GuiPrivate がない、とふたたびビルドエラー。
  • しかし、単独のコンポーネントは存在せず、Qt本体にあるべきもの。
  • ダウンロードエラーを疑い、Qt 6.8.3 本体を消して再インストールするが同じエラーが出る。
  • 他の開発者はどうしているのか、などとChatGPTと壁打ちするうち、GitHubにあるMuseScore Wiki によると、開発版で使用する Qt バージョンは 6.9.3 以上と判明。
  • DCMAKE_PREFIX_PATH で 6.5.3 のままにしておりビルドエラー。修正して先へ進む。

ビルド時間が長すぎる問題

  • ファイル単位で書き換えた後、正しく動くか検証するためにビルド。しかし、1回ビルドするたび十数分～30分程度掛かってしまって作業が進まない。
  • 次の代替案はあるらしい。（ただし情報収集のみ、未検証）
    • 編集するファイルを Unity Build から外す
    • cmake の build オプションで、RelWithDebInfo → Debug モードに変更
  • ビルド並列度を設定する -j
    • 旧PCでは限界迎えて結局変わらず。

そのほか

  • QML 用のシンタックスハイライト（拡張機能）を導入

Gitbash 操作

状況確認

  • origin：自分のGitHubの写し
  • upstream：本家 MuseScore のソース
  • master : 手元のコピー
git remote -v

//出力例
origin  git@github.com:pyonpyoco/MuseScore.git (fetch)
origin  git@github.com:pyonpyoco/MuseScore.git (push)
upstream        https://github.com/musescore/MuseScore.git (fetch)
upstream        https://github.com/musescore/MuseScore.git (push)

ローカルを最新にする

  • push もこまめにしておくと良い。
    • PR作業は origin からブランチを切って作るため、origin (fork) が本家とズレたままだと後々面倒になる。
//リポジトリのフォルダへ移動
cd ./git/MuseScore


//現在地の確認
git branch

//出力例
* feature/fret-tp-fingering
  master

//別ブランチにいる場合、masterに移動
git checkout master

//最新情報を取得、masterに最新情報を取り込む、gitHubの自分のアカウントと同期→ローカル・fork・本家が揃う
git fetch upstream
git pull upstream master
git push origin master

作業用ブランチの処理

  • 必ず最新の master からブランチを切るのが原則
    • 古い master で作業すると、PRが conflict だらけになる
  • ローカルリポジトリで、作業専用ラインを作る。
  • 作業用ブランチを、自分のGitHubアカウントに push し、一度はブランチ登録する。
  • VSCode も Git を見て動くので、ブランチを切ると VSCode でもほぼ自動で同期される。
//newBranchA のブランチを切って作業用にする
git checkout -b newBranchA

//ブランチをはじめてGitHubに押し上げる
git push -u origin newBranchA

// -uを付けておくと便利。次のコマンドだけでそのブランチが自動で対象になる
git push
git pull

PR のためにやること

  • 作業用ブランチに master の最新状態を取り込む (rebase)
  • コードを commit → push
    • commit : 自分のPCのなかで変更を記録する（セーブ）
    • push : その記録をGitHubに送る（アップロード）
  • GitHub で PR 作成、説明文を書く
  • 更新する場合は、こまめにcommit→push すると他者から見て変更点がわかりやすい
    • push すると、GitHub が変更を自動的に検知して、PR でも情報が更新される。
    • PR が fork 内の作業ブランチを参照しており、最新 commit を常に覗いている。そのたびに CI が自動的に走る。
// 作業用ブランチの変更を commit
// 現在の branch の変更点を確認
git diff
git add .
git commit -m "add xxxxx"

// あらかじめ、master を upstream と同期させておく。そのうえで…
git checkout newBranchA
git rebase master

// GitHub (origin) へ push
git push origin newBranchA
// すでに push していて、rebase も含む場合
git push origin newBranchA --force-with-lease

調べもの

  • Qt とはなにか
    • クロスプラットフォームGUIアプリ開発のための巨大フレームワーク
    • Windows, Mac, Linux, Android, iOS すべてで動くアプリをほぼ共通のC++で書ける。
    • QtCore : スレッド、イベントループ、文字列、日付、データ構造、ファイル I/O…
    • QtWidgets / QtQuick : ウィンドウ、ボタン、描画、アニメーション、レイアウト
    • QML : モダン UI 向けの宣言的 UI 言語（スマホアプリ的UIを作りやすい）
    • QtNetwork / QtMultimedia / QtSQL : ネット通信、音声、動画、DB 接続…
    • .ui : QtWidgets (XML → C++)
    • .qml : QtQuick (宣言的UI)
    • Signal / Slot の独自モデル：信号（発生したイベント）→スロット（それを受けて動く処理）
  • MSVC構成 vs MinGW
    • Microsoft Visual C++（=MSVC）コンパイラ用にビルドされた Qt のセット
    • cf. MinGW ：オープンソースのGCCベース
    • デバッガに MinGW を使っていたみたいだが、MSVCでもデバッグ可能らしい。cppvsdbg
タイトルとURLをコピーしました