Qt: Implement Show Platforms / Show Regions #5786

grimpunch · 2017-07-14T09:59:15Z

Porting the View config logic from WX to Qt for Show Platforms & Show Regions.
Adds to the View Menu.

sepalani

The lint errors also need to be fixed.

Source/Core/DolphinQt2/GameList/TableProxyModel.cpp

@@ -0,0 +1,19 @@
+// Copyright 2015 Dolphin Emulator Project


Source/Core/DolphinQt2/GameList/TableProxyModel.h

@@ -0,0 +1,16 @@
+// Copyright 2015 Dolphin Emulator Project


Source/Core/DolphinQt2/GameList/GameListModel.cpp

  beginInsertRows(QModelIndex(), entry, entry);
  m_games.insert(entry, game);
-  endInsertRows();
+  endInsertRows(); 


grimpunch · 2017-07-14T14:03:25Z

The above issues are fixed now, @sepalani

sepalani

Could you also please squash your commits into a single one?

Source/Core/DolphinQt2/GameList/GameListModel.cpp

+bool GameListModel::ShouldDisplayGameListItem(const QModelIndex& index) {
+  QSharedPointer<GameFile> game = m_games[index.row()];
+
+


Source/Core/DolphinQt2/GameList/GameListModel.cpp

@@ -148,6 +205,7 @@ void GameListModel::UpdateGame(QSharedPointer<GameFile> game)
  else
    return;

+


Source/Core/DolphinQt2/GameList/TableProxyModel.h

+public:
+  explicit TableProxyModel(QObject* parent = nullptr);
+  bool filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const override;
+


spycrab

Apart from the small mistakes here and there and the Linter error, looks pretty good to me.

Definitely well done for a first-time contribution!

Source/Core/DolphinQt2/GameList/TableProxyModel.cpp

+bool TableProxyModel::filterAcceptsRow(int sourceRow,
+  const QModelIndex &sourceParent) const
+{
+  GameListModel *glm = qobject_cast<GameListModel *>(sourceModel());


Source/Core/DolphinQt2/GameList/ListProxyModel.cpp

+bool ListProxyModel::filterAcceptsRow(int sourceRow,
+  const QModelIndex &sourceParent) const
+{
+  GameListModel *glm = qobject_cast<GameListModel *>(sourceModel());


Source/Core/DolphinQt2/GameList/GameListModel.cpp

+  QSharedPointer<GameFile> game = m_games[index.row()];
+
+
+  const bool show_platform = [&game] {


Source/Core/DolphinQt2/GameList/ListProxyModel.h

@@ -13,4 +13,5 @@ class ListProxyModel final : public QSortFilterProxyModel
 public:
  explicit ListProxyModel(QObject* parent = nullptr);
  QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
+  bool filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const override;


Source/Core/DolphinQt2/GameList/TableProxyModel.h

+
+public:
+  explicit TableProxyModel(QObject* parent = nullptr);
+  bool filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const override;


Source/Core/DolphinQt2/MenuBar.cpp

@@ -270,6 +273,66 @@ void MenuBar::AddTableColumnsMenu(QMenu* view_menu)
  }
 }

+void MenuBar::AddShowPlatformsMenu(QMenu* view_menu)
+{
+  static const QMap<QString, bool*> platform_menu{


Source/Core/DolphinQt2/MenuBar.cpp

+    {tr("Show ELF/DOL"), &SConfig::GetInstance().m_ListElfDol}};
+
+  QActionGroup* platform_group = new QActionGroup(this);
+  QMenu* plat_menu = view_menu->addMenu(tr("Show Platforms"));


Source/Core/DolphinQt2/MainWindow.cpp

@@ -195,6 +195,10 @@ void MainWindow::ConnectMenuBar()
  connect(m_menu_bar, &MenuBar::ShowList, m_game_list, &GameList::SetListView);
  connect(m_menu_bar, &MenuBar::ColumnVisibilityToggled, m_game_list,
          &GameList::OnColumnVisibilityToggled);
+  connect(m_menu_bar, &MenuBar::GameListPlatformVisibilityToggled, m_game_list,
+    &GameList::OnGameListPlatformVisibilityToggled);
+  connect(m_menu_bar, &MenuBar::GameListRegionVisibilityToggled, m_game_list,


Source/Core/DolphinQt2/GameList/GameList.cpp

+  m_list_proxy->invalidate();
+}
+
+void GameList::OnGameListRegionVisibilityToggled()


mimimi085181 · 2017-07-14T15:37:07Z

This pr seems to break being able to sort the gamelist by platform.

The JAP, PAL and USA are seperated from the rest on wx, because those are the actual main regions. If you copy wx 1:1, you need to seperate them the same way. Adding Korea to those 3 would make sense though, since that's the 4th known possible region of wiis.

It might be a good idea to rebase the pr on top of latest master, because master contains a change related to the gamelist sorting. If you don't know how to do that, ask on irc, or look at my cheat sheet:

Link your upstream to the official master(only once):
git remote add upstream https://github.com/dolphin-emu/dolphin.git

Rebase on top of latest master:
git pull --rebase upstream master

JosJuice · 2017-07-14T15:46:21Z

The JAP, PAL and USA are seperated from the rest on wx, because those are the actual main regions. If you copy wx 1:1, you need to seperate them the same way. Adding Korea to those 3 would make sense though, since that's the 4th known possible region of wiis.

It doesn't make much sense that DolphinWX does that, because it's not actually filtering by region. (For instance, if you have a PAL game that shows up with a French flag, it doesn't show up if only PAL is selected, because "PAL" means "Europe flag" to DolphinWX.) I'm fine with doing a 1:1 copy of wx for now, though.

grimpunch · 2017-07-14T16:21:53Z

Thanks for the advice. I am away for the weekend now so I will rebase my changes onto latest master , incorporate @spycrab 's suggestions and then push everything up on Monday or Tuesday :)

Source/Core/DolphinQt2/GameList/GameListModel.cpp

+    default:
+      return false;
+    }
+  }();


Source/Core/DolphinQt2/GameList/GameListModel.cpp

@@ -138,6 +138,63 @@ int GameListModel::columnCount(const QModelIndex& parent) const
  return NUM_COLS;
 }

+bool GameListModel::ShouldDisplayGameListItem(const QModelIndex& index) {


Source/Core/DolphinQt2/MenuBar.h

@@ -50,6 +50,8 @@ class MenuBar final : public QMenuBar
  void ShowTable();
  void ShowList();
  void ColumnVisibilityToggled(const QString& row, bool visible);
+  void GameListPlatformVisibilityToggled(const QString& row, bool visible);
+  void GameListRegionVisibilityToggled(const QString& row, bool visible);


ligfx · 2017-07-14T21:55:10Z

Looks pretty good, couple comments.

I do wonder if we might want to extract the filterAcceptsRow logic into it's own proxy model that the others pull from (e.g. list and table proxy models <- region/platform filter proxy model <- game list model), but that's not necessary right now.

MayImilae

The design looks good! The way WX has three separate has already been mentioned, but it doesn't quite work so I don't mind that it isn't here. So I'm going to go ahead and approve!

grimpunch · 2017-07-17T11:56:06Z

Rebased after making requested changes and squashing them.

grimpunch · 2017-07-17T17:15:41Z

Addressed linting errors, amended commit.

grimpunch · 2017-07-18T08:13:26Z

@spycrab @sepalani , I think everything is okay now if you have a chance to look over it. 👍

ligfx

Two additional code comments.

Also, the lint buildbot doesn't seem to be working correctly: it says "Invalid symmetric difference expression bcdab8a..." but returns successfully :/

EDIT: Okay, it worked properly after a rebuild. I guess it was some transitory error.

Source/Core/DolphinQt2/GameList/GameList.cpp

@@ -34,9 +35,7 @@ static bool CompressCB(const std::string&, float, void*);
 GameList::GameList(QWidget* parent) : QStackedWidget(parent)
 {
  m_model = new GameListModel(this);
-  m_table_proxy = new QSortFilterProxyModel(this);
-  m_table_proxy->setSortCaseSensitivity(Qt::CaseInsensitive);
-  m_table_proxy->setSortRole(Qt::InitialSortOrderRole);


Source/Core/DolphinQt2/GameList/TableProxyModel.cpp

+TableProxyModel::TableProxyModel(QObject* parent) : QSortFilterProxyModel(parent)
+{
+  setSortCaseSensitivity(Qt::CaseInsensitive);
+  sort(GameListModel::COL_TITLE);


spycrab

If you make the linter happy, good to go by me:

!!! Source/Core/DolphinQt2/MenuBar.cpp not compliant to coding style, here is the fix:
--- Source/Core/DolphinQt2/MenuBar.cpp	2017-07-18 10:21:02.321969281 +0000
+++ /dev/fd/63	2017-07-18 10:21:02.857974465 +0000
@@ -315,7 +315,7 @@
       {tr("Show Taiwan"), &SConfig::GetInstance().m_ListTaiwan},
       {tr("Show World"), &SConfig::GetInstance().m_ListWorld},
       {tr("Show Unknown"), &SConfig::GetInstance().m_ListUnknown}};
-  
+
   QActionGroup* region_group = new QActionGroup(this);
   QMenu* region_menu = view_menu->addMenu(tr("Show Regions"));
   region_group->setExclusive(false);

grimpunch · 2017-07-18T22:12:38Z

Thanks @spycrab , think linter should be happy with this one 🤞

grimpunch · 2017-07-19T09:43:43Z

@ligfx & @sepalani , is it okay with you two?

ligfx

One more thing, then it's fine with me.

Source/Core/DolphinQt2/GameList/ListProxyModel.cpp

@@ -34,3 +34,9 @@ QVariant ListProxyModel::data(const QModelIndex& i, int role) const
  }
  return QVariant();
 }
+
+bool ListProxyModel::filterAcceptsRow(int sourceRow, const QModelIndex& sourceParent) const


sepalani · 2017-07-19T22:17:31Z

I'm not a Qt guy but besides what @ligfx mentioned for both filterAcceptsRow methods (which should also be changed in the header files) SGTM.

grimpunch · 2017-07-20T06:43:03Z

@ligfx , I've amended the commit. :)

Source/Core/DolphinQt2/GameList/GameListModel.h

@@ -27,6 +27,7 @@ class GameListModel final : public QAbstractTableModel

  // Path of the Game at the specified index.
  QString GetPath(int index) const { return m_games[index]->GetFilePath(); }
+  bool ShouldDisplayGameListItem(const int index);


Porting the View config logic from WX to Qt. Adds to the View Menu

grimpunch · 2017-07-24T13:50:02Z

@leoetlino , I changed the signature of ShouldDisplayGameListItem

grimpunch · 2017-07-25T08:56:28Z

I'm happy with the state of this PR now, just for the record of whoever is merging it. 👍

sepalani suggested changes Jul 14, 2017

View reviewed changes

spycrab requested changes Jul 14, 2017

View reviewed changes

ligfx reviewed Jul 14, 2017

View reviewed changes

Source/Core/DolphinQt2/GameList/GameListModel.cpp Outdated

@@ -138,6 +138,63 @@ int GameListModel::columnCount(const QModelIndex& parent) const

return NUM_COLS;

}

bool GameListModel::ShouldDisplayGameListItem(const QModelIndex& index) {

This comment was marked as off-topic.

Sign in to view

ligfx reviewed Jul 14, 2017

View reviewed changes

MayImilae approved these changes Jul 15, 2017

View reviewed changes

grimpunch force-pushed the qt_viewoptions branch 2 times, most recently from ba84e21 to 6364c0f Compare July 17, 2017 11:55

grimpunch force-pushed the qt_viewoptions branch from 6364c0f to 1ad5c78 Compare July 17, 2017 17:15

ligfx suggested changes Jul 18, 2017

View reviewed changes

grimpunch force-pushed the qt_viewoptions branch from 1ad5c78 to 84a71bf Compare July 18, 2017 09:23

spycrab approved these changes Jul 18, 2017

View reviewed changes

grimpunch force-pushed the qt_viewoptions branch from 84a71bf to 894e6d7 Compare July 18, 2017 22:12

ligfx reviewed Jul 19, 2017

View reviewed changes

grimpunch force-pushed the qt_viewoptions branch from 894e6d7 to 7e3940b Compare July 19, 2017 23:15

ligfx approved these changes Jul 23, 2017

View reviewed changes

leoetlino reviewed Jul 24, 2017

View reviewed changes

Qt: Implement Show Platforms / Show Regions

df35a26

Porting the View config logic from WX to Qt. Adds to the View Menu

grimpunch force-pushed the qt_viewoptions branch from 7e3940b to df35a26 Compare July 24, 2017 13:36

leoetlino merged commit be83243 into dolphin-emu:master Jul 26, 2017

grimpunch deleted the qt_viewoptions branch July 26, 2017 09:26

leoetlino modified the milestone: Qt Sep 13, 2017

		bool GameListModel::ShouldDisplayGameListItem(const QModelIndex& index) {
		QSharedPointer<GameFile> game = m_games[index.row()];

		@@ -148,6 +205,7 @@ void GameListModel::UpdateGame(QSharedPointer<GameFile> game)
		else
		return;

		QSharedPointer<GameFile> game = m_games[index.row()];


		const bool show_platform = [&game] {

Qt: Implement Show Platforms / Show Regions #5786

Qt: Implement Show Platforms / Show Regions #5786

Conversation

grimpunch commented Jul 14, 2017

sepalani left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

grimpunch commented Jul 14, 2017

sepalani left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

spycrab left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

mimimi085181 commented Jul 14, 2017

JosJuice commented Jul 14, 2017

grimpunch commented Jul 14, 2017

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

ligfx commented Jul 14, 2017

MayImilae left a comment

Choose a reason for hiding this comment

grimpunch commented Jul 17, 2017

grimpunch commented Jul 17, 2017

grimpunch commented Jul 18, 2017

ligfx left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

spycrab left a comment

Choose a reason for hiding this comment

grimpunch commented Jul 18, 2017

grimpunch commented Jul 19, 2017

ligfx left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

sepalani commented Jul 19, 2017

grimpunch commented Jul 20, 2017

This comment was marked as off-topic.

grimpunch commented Jul 24, 2017

grimpunch commented Jul 25, 2017

ligfx left a comment •

edited

Loading