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.

Sign up for GitHub

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

Jump to bottom

CClipboard のテストにモックを導入する #1800

Merged
merged 5 commits into from
Feb 14, 2022
Merged

CClipboard のテストにモックを導入する #1800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
733f16e
CClipboard.h 編集時の再コンパイル範囲を縮小する。
kengoide Feb 12, 2022
e7a0b5c
CClipboard 内の API 呼び出しをオーバーライドできるようにする。
kengoide Feb 13, 2022
a5a85a8
CClipboard のテストで Google Mock を使用する。
kengoide Feb 13, 2022
eca61cc
protected コンストラクターを explicit にする
kengoide Feb 13, 2022
d2a0f72
レビューコメントを反映
kengoide Feb 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions sakura_core/_os/CClipboard.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ CClipboard::~CClipboard()

void CClipboard::Empty()
{
::EmptyClipboard();
EmptyClipboard();
Copy link
Contributor

Choose a reason for hiding this comment

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

ぬおっ！

やり方色々あるっす。

方法 メリット デメリット
APIを自クラスのメンバとして定義する 書き換えが楽。 自クラスのメンバーがけっこう増える。
APIラッパークラスを定義してGetする 自クラスのメンバー追加は1個で済む。 ラッパークラスの定義内容でモメるかも。
APIラッパークラスを定義してシングルトンにするする 自クラスのメンバーは増えない。 ラッパークラスの定義内容でモメるかも。

たとえばこんな感じです。

CClipboardApi::getInstance()->EmptyClipboard();

}

void CClipboard::Close()
Expand Down Expand Up @@ -112,7 +112,7 @@ bool CClipboard::SetText(
::GlobalUnlock( hgClipText );

//クリップボードに設定
::SetClipboardData( CF_UNICODETEXT, hgClipText );
SetClipboardData( CF_UNICODETEXT, hgClipText );
bUnicodeText = false;
}
// 1回しか通らない. breakでここまで飛ぶ
Expand Down Expand Up @@ -142,7 +142,7 @@ bool CClipboard::SetText(
::GlobalUnlock( hgClipSakura );

//クリップボードに設定
::SetClipboardData( uFormatSakuraClip, hgClipSakura );
SetClipboardData( uFormatSakuraClip, hgClipSakura );
bSakuraText = false;
}
// 1回しか通らない. breakでここまで飛ぶ
Expand All @@ -160,7 +160,7 @@ bool CClipboard::SetText(
BYTE* pClip = GlobalLockBYTE( hgClipMSDEVColumn );
pClip[0] = 0;
::GlobalUnlock( hgClipMSDEVColumn );
::SetClipboardData( uFormat, hgClipMSDEVColumn );
SetClipboardData( uFormat, hgClipMSDEVColumn );
}
}
}
Expand All @@ -178,7 +178,7 @@ bool CClipboard::SetText(
BYTE* pClip = (BYTE*)::GlobalLock( hgClipMSDEVLine );
pClip[0] = 0x01;
::GlobalUnlock( hgClipMSDEVLine );
::SetClipboardData( uFormat, hgClipMSDEVLine );
SetClipboardData( uFormat, hgClipMSDEVLine );
}
}
}
Expand All @@ -194,7 +194,7 @@ bool CClipboard::SetText(
BYTE* pClip = (BYTE*)::GlobalLock( hgClipMSDEVLine2 );
pClip[0] = 0x01; // ※ ClipSpy で調べるとデータはこれとは違うが内容には無関係に動くっぽい
::GlobalUnlock( hgClipMSDEVLine2 );
::SetClipboardData( uFormat, hgClipMSDEVLine2 );
SetClipboardData( uFormat, hgClipMSDEVLine2 );
}
}
}
Expand Down Expand Up @@ -250,7 +250,7 @@ bool CClipboard::SetHtmlText(const CNativeW& cmemBUf)

//クリップボードに設定
UINT uFormat = ::RegisterClipboardFormat( L"HTML Format" );
::SetClipboardData( uFormat, hgClipText );
SetClipboardData( uFormat, hgClipText );
return true;
}

Expand Down Expand Up @@ -299,8 +299,8 @@ bool CClipboard::GetText(CNativeW* cmemBuf, bool* pbColumnSelect, bool* pbLineSe
//サクラ形式のデータがあれば取得
CLIPFORMAT uFormatSakuraClip = CClipboard::GetSakuraFormat();
if( (uGetFormat == -1 || uGetFormat == uFormatSakuraClip)
&& ::IsClipboardFormatAvailable( uFormatSakuraClip ) ){
HGLOBAL hSakura = ::GetClipboardData( uFormatSakuraClip );
&& IsClipboardFormatAvailable( uFormatSakuraClip ) ){
HGLOBAL hSakura = GetClipboardData( uFormatSakuraClip );
if (hSakura != NULL) {
BYTE* pData = (BYTE*)::GlobalLock(hSakura);
size_t nLength = *((int*)pData);
Expand All @@ -315,7 +315,7 @@ bool CClipboard::GetText(CNativeW* cmemBuf, bool* pbColumnSelect, bool* pbLineSe
// From Here 2005/05/29 novice UNICODE TEXT 対応処理を追加
HGLOBAL hUnicode = NULL;
if( uGetFormat == -1 || uGetFormat == CF_UNICODETEXT ){
hUnicode = ::GetClipboardData( CF_UNICODETEXT );
hUnicode = GetClipboardData( CF_UNICODETEXT );
}
if( hUnicode != NULL ){
//DWORD nLen = GlobalSize(hUnicode);
Expand All @@ -329,7 +329,7 @@ bool CClipboard::GetText(CNativeW* cmemBuf, bool* pbColumnSelect, bool* pbLineSe
//OEMTEXT形式のデータがあれば取得
HGLOBAL hText = NULL;
if( uGetFormat == -1 || uGetFormat == CF_OEMTEXT ){
hText = ::GetClipboardData( CF_OEMTEXT );
hText = GetClipboardData( CF_OEMTEXT );
}
if( hText != NULL ){
char* szData = GlobalLockChar(hText);
Expand All @@ -348,8 +348,8 @@ bool CClipboard::GetText(CNativeW* cmemBuf, bool* pbColumnSelect, bool* pbLineSe
/* 2008.09.10 bosagami パス貼り付け対応 */
//HDROP形式のデータがあれば取得
if( (uGetFormat == -1 || uGetFormat == CF_HDROP)
&& ::IsClipboardFormatAvailable(CF_HDROP) ){
HDROP hDrop = (HDROP)::GetClipboardData(CF_HDROP);
&& IsClipboardFormatAvailable(CF_HDROP) ){
HDROP hDrop = (HDROP)GetClipboardData(CF_HDROP);
if(hDrop != NULL){
WCHAR sTmpPath[_MAX_PATH + 1] = {0};
const int nMaxCnt = DragQueryFile(hDrop, 0xFFFFFFFF, NULL, 0);
Expand Down Expand Up @@ -673,3 +673,19 @@ int CClipboard::GetDataType()
if(::IsClipboardFormatAvailable(CF_HDROP))return CF_HDROP;
return -1;
}

HANDLE CClipboard::SetClipboardData(UINT uFormat, HANDLE hMem) const {
return ::SetClipboardData(uFormat, hMem);
Copy link
Contributor

Choose a reason for hiding this comment

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

このAPI関数ですが、クラスメンバを利用しないのでconst関数とすべきだと思います。

他のAPI関数も同様です。

Copy link
Member Author

Choose a reason for hiding this comment

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

const にしました。

}

HANDLE CClipboard::GetClipboardData(UINT uFormat) const {
return ::GetClipboardData(uFormat);
}

BOOL CClipboard::EmptyClipboard() const {
return ::EmptyClipboard();
}

BOOL CClipboard::IsClipboardFormatAvailable(UINT format) const {
return ::IsClipboardFormatAvailable(format);
}
11 changes: 11 additions & 0 deletions sakura_core/_os/CClipboard.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,5 +66,16 @@ class CClipboard{
static bool HasValidData(); //!< クリップボード内に、サクラエディタで扱えるデータがあればtrue
static CLIPFORMAT GetSakuraFormat(); //!< サクラエディタ独自のクリップボードデータ形式
static int GetDataType(); //!< クリップボードデータ形式(CF_UNICODETEXT等)の取得

protected:
// 単体テスト用コンストラクタ
explicit CClipboard(bool openStatus) : m_bOpenResult(openStatus) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

ここにexplicitを付けるなら、元々定義されているコンストラクタにも付けるべきと思いました。

コメントで書きましたが元のコンストラクタがおかしいので、ここに追加したコンストラクタは将来的に削除されることになると思っています。

Copy link
Member Author

Choose a reason for hiding this comment

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

既存のコードの改善を始める前にテストを整備したいので元のコンストラクタには手を付けていません。

失敗の可能性があるコンストラクタは static なファクトリー関数に置き換えるのが良いと考えています。クリップボードのオープンに失敗したら即座に呼び出し元に通知するべきです。状態を持つ必要がなくなるのでクラス内の条件分岐が減らせます。

このあたりの設計については各論あると思いますのでまたの機会に詳しく検討させてください。


// 同名の Windows API に引数を転送する仮想メンバ関数。
// 単体テスト内でオーバーライドすることで副作用のないテストを実施するのが目的。
virtual HANDLE SetClipboardData(UINT uFormat, HANDLE hMem) const;
virtual HANDLE GetClipboardData(UINT uFormat) const;
virtual BOOL EmptyClipboard() const;
virtual BOOL IsClipboardFormatAvailable(UINT format) const;
};
#endif /* SAKURA_CCLIPBOARD_4E783022_214C_4E51_A2E0_54EC343500F6_H_ */
1 change: 1 addition & 0 deletions sakura_core/cmd/CViewCommander_Clipboard.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "uiparts/CWaitCursor.h"
#include "util/os.h"
#include "apiwrap/CommonControl.h"
#include "_os/CClipboard.h"

/** 切り取り(選択範囲をクリップボードにコピーして削除)

Expand Down
6 changes: 6 additions & 0 deletions sakura_core/doc/CDocEditor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "CEol.h"
#include "window/CEditWnd.h"
#include "debug/CRunningTimer.h"
#include "_os/CClipboard.h"

CDocEditor::CDocEditor(CEditDoc* pcDoc)
: m_pcDocRef(pcDoc)
Expand Down Expand Up @@ -166,6 +167,11 @@ void CDocEditor::SetImeMode( int mode )
}
// To Here Nov. 20, 2000 genta

bool CDocEditor::IsEnablePaste() const
{
return CClipboard::HasValidData();
}

/*!
末尾に行を追加

Expand Down
6 changes: 1 addition & 5 deletions sakura_core/doc/CDocEditor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
#pragma once

#include "doc/CDocListener.h"
#include "_os/CClipboard.h"
#include "COpeBuf.h"

class CEditDoc;
Expand Down Expand Up @@ -84,10 +83,7 @@ class CDocEditor : public CDocListenerEx{
}

//! クリップボードから貼り付け可能か？
bool IsEnablePaste( void ) const
{
return CClipboard::HasValidData();
}
bool IsEnablePaste( void ) const;

public:
CEditDoc* m_pcDocRef;
Expand Down
1 change: 1 addition & 0 deletions sakura_core/macro/CMacro.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
#include "CSelectLang.h"
#include "config/app_constants.h"
#include "String_define.h"
#include "_os/CClipboard.h"

CMacro::CMacro( EFunctionCode nFuncID )
{
Expand Down
Loading