Workaround sfml image loading thread race condition

See: https://github.com/SFML/SFML/issues/1711

Also some other smaller changes

12 files changed, 97 insertions, 47 deletions

diff --git a/TODO b/TODO
index 6d75453..2da7a9c 100644
--- a/TODO
+++ b/TODO
@@ -108,4 +108,5 @@ Disable posting in 4chan thread if closed (thread json contains "closed" field f
 Remove calls to get the original message of an edit in edits and replies in matrix if possible. These calls take additional time, and with a slow homeserver or high ping this could make messages to be delayed by an annoying amount of time.
 Read image exif into to apply image rotation. This is common in images taken on phones. If not done, the width and height will also be mixed and thumbnail fallback size will be incorrectly calculated (for example in matrix).
 Handle M_LIMIT_EXCEEDED in matrix
-Check if we need to call /_matrix/client/r0/notifications for intial sync to receive old notifications or if /sync always includes mentions.
\ No newline at end of file
+Check if we need to call /_matrix/client/r0/notifications for intial sync to receive old notifications or if /sync always includes mentions.
+Maybe dont clear cache for body items when filtering.
\ No newline at end of file
diff --git a/include/Body.hpp b/include/Body.hpp
index 7bf89e2..baed595 100644
--- a/include/Body.hpp
+++ b/include/Body.hpp
@@ -209,7 +209,7 @@ namespace QuickMedia {
         bool draw_thumbnails;
         bool wrap_around;
         // Set to {0, 0} to disable resizing
-        sf::Vector2i thumbnail_resize_target_size;
+        sf::Vector2i thumbnail_max_size;
         sf::Color line_separator_color;
         BodyItemRenderCallback body_item_render_callback;
         sf::Shader *thumbnail_mask_shader;
diff --git a/include/SfmlFixes.hpp b/include/SfmlFixes.hpp
new file mode 100644
index 0000000..fc893c5
--- /dev/null
+++ b/include/SfmlFixes.hpp
@@ -0,0 +1,11 @@
+#pragma once
+
+#include <SFML/Graphics/Image.hpp>
+
+namespace QuickMedia {
+    // See: https://github.com/SFML/SFML/issues/1711
+    // TODO: Remove when above is fixed, or fix ourselves and include the fixed sfml version as a dependency,
+    // or write our own image class (maybe even use stb like sfml does but no error reason printing)
+    bool load_image_from_file(sf::Image &image, const std::string &filepath);
+    bool load_image_from_memory(sf::Image &image, const void *data, size_t size);
+}
\ No newline at end of file
diff --git a/plugins/Mangadex.hpp b/plugins/Mangadex.hpp
index 9990792..1b238a2 100644
--- a/plugins/Mangadex.hpp
+++ b/plugins/Mangadex.hpp
@@ -11,7 +11,7 @@ namespace QuickMedia {
         bool search_is_filter() override { return false; }
         SearchResult search(const std::string &str, BodyItems &result_items) override;
         PluginResult submit(const std::string &title, const std::string &url, std::vector<Tab> &result_tabs) override;
-        sf::Vector2i get_max_thumbnail_size() override { return sf::Vector2i(101, 141); };
+        sf::Vector2i get_thumbnail_max_size() override { return sf::Vector2i(101, 141); };
     private:
         bool get_rememberme_token(std::string &rememberme_token);
         std::optional<std::string> rememberme_token;
diff --git a/plugins/Manganelo.hpp b/plugins/Manganelo.hpp
index b82476c..4fbce5b 100644
--- a/plugins/Manganelo.hpp
+++ b/plugins/Manganelo.hpp
@@ -11,7 +11,7 @@ namespace QuickMedia {
         bool search_is_filter() override { return false; }
         SearchResult search(const std::string &str, BodyItems &result_items) override;
         PluginResult submit(const std::string &title, const std::string &url, std::vector<Tab> &result_tabs) override;
-        sf::Vector2i get_max_thumbnail_size() override { return sf::Vector2i(101, 141); };
+        sf::Vector2i get_thumbnail_max_size() override { return sf::Vector2i(101, 141); };
     private:
         bool extract_id_from_url(const std::string &url, std::string &manga_id) const;
     };
diff --git a/plugins/Mangatown.hpp b/plugins/Mangatown.hpp
index 0062d51..10f2500 100644
--- a/plugins/Mangatown.hpp
+++ b/plugins/Mangatown.hpp
@@ -12,7 +12,7 @@ namespace QuickMedia {
         SearchResult search(const std::string &str, BodyItems &result_items) override;
         PluginResult get_page(const std::string &str, int page, BodyItems &result_items) override;
         PluginResult submit(const std::string &title, const std::string &url, std::vector<Tab> &result_tabs) override;
-        sf::Vector2i get_max_thumbnail_size() override { return sf::Vector2i(101, 141); };
+        sf::Vector2i get_thumbnail_max_size() override { return sf::Vector2i(101, 141); };
     private:
         bool extract_id_from_url(const std::string &url, std::string &manga_id) const;
     };
diff --git a/plugins/Page.hpp b/plugins/Page.hpp
index fb828fb..f65cd21 100644
--- a/plugins/Page.hpp
+++ b/plugins/Page.hpp
@@ -41,7 +41,7 @@ namespace QuickMedia {
 
         bool load_manga_content_storage(const char *service_name, const std::string &manga_title, const std::string &manga_id);
 
-        virtual sf::Vector2i get_max_thumbnail_size() { return sf::Vector2i(480, 360); };
+        virtual sf::Vector2i get_thumbnail_max_size() { return sf::Vector2i(480, 360); };
     
         Program *program;
     };
diff --git a/src/AsyncImageLoader.cpp b/src/AsyncImageLoader.cpp
index ec9ae9a..98c7fee 100644
--- a/src/AsyncImageLoader.cpp
+++ b/src/AsyncImageLoader.cpp
@@ -2,23 +2,14 @@
 #include "../include/DownloadUtils.hpp"
 #include "../include/ImageUtils.hpp"
 #include "../include/Scale.hpp"
+#include "../include/SfmlFixes.hpp"
 #include "../external/hash-library/sha256.h"
 #include <assert.h>
 
 namespace QuickMedia {
-    static sf::Vector2f to_vec2f(const sf::Vector2u &vec) {
-        return sf::Vector2f(vec.x, vec.y);
-    }
-
-    static sf::Vector2f to_vec2f(const sf::Vector2i &vec) {
-        return sf::Vector2f(vec.x, vec.y);
-    }
-
-    static sf::Vector2u to_vec2u(const sf::Vector2f &vec) {
-        return sf::Vector2u(vec.x, vec.y);
-    }
-
     // Linear interpolation
+    // TODO: Is this implementation ok? it always uses +1 offset for interpolation but if we were to resize an image with near 1x1 scaling
+    // then it would be slightly blurry
     static void copy_resize(const sf::Image &source, sf::Image &destination, sf::Vector2u destination_size) {
         const sf::Vector2u source_size = source.getSize();
         if(source_size.x == 0 || source_size.y == 0 || destination_size.x == 0 || destination_size.y == 0)
@@ -120,14 +111,14 @@ namespace QuickMedia {
                 }
 
                 thumbnail_load_data.thumbnail_data->image = std::make_unique<sf::Image>();
-                if(thumbnail_load_data.thumbnail_data->image->loadFromFile(thumbnail_load_data.thumbnail_path.data)) {
+                if(load_image_from_file(*thumbnail_load_data.thumbnail_data->image, thumbnail_load_data.thumbnail_path.data)) {
                     fprintf(stderr, "Loaded %s from thumbnail cache\n", thumbnail_load_data.path.data.c_str());
                     thumbnail_load_data.thumbnail_data->loading_state = LoadingState::FINISHED_LOADING;
                     continue;
                 }
 
                 if(thumbnail_load_data.local) {
-                    if(thumbnail_load_data.thumbnail_data->image->loadFromFile(thumbnail_load_data.path.data)
+                    if(load_image_from_file(*thumbnail_load_data.thumbnail_data->image, thumbnail_load_data.path.data)
                         && thumbnail_load_data.resize_target_size.x != 0 && thumbnail_load_data.resize_target_size.y != 0)
                     {
                         create_thumbnail_if_thumbnail_smaller_than_image(thumbnail_load_data.path.data, thumbnail_load_data.thumbnail_path, thumbnail_load_data.thumbnail_data.get(), thumbnail_load_data.resize_target_size);
@@ -194,21 +185,21 @@ namespace QuickMedia {
         // TODO: Keep the thread running and use conditional variable instead to sleep until a new image should be loaded. Same in ImageViewer.
         download_image_thread[free_index] = std::thread([this, free_index, thumbnail_path, url, local, resize_target_size, thumbnail_data, use_tor]() mutable {
             thumbnail_data->image = std::make_unique<sf::Image>();
-            if(thumbnail_data->image->loadFromFile(thumbnail_path.data)) {
+            if(load_image_from_file(*thumbnail_data->image, thumbnail_path.data)) {
                 fprintf(stderr, "Loaded %s from thumbnail cache\n", url.c_str());
                 thumbnail_data->loading_state = LoadingState::FINISHED_LOADING;
                 loading_image[free_index] = false;
                 return;
             } else {
                 if(local) {
-                    if(!thumbnail_data->image->loadFromFile(url)) {
+                    if(!load_image_from_file(*thumbnail_data->image, url)) {
                         thumbnail_data->loading_state = LoadingState::FINISHED_LOADING;
                         loading_image[free_index] = false;
                         return;
                     }
                 } else {
                     // Use the same path as the thumbnail path, since it wont be overwritten if the image is smaller than the thumbnail target size
-                    if(download_to_file(url, thumbnail_path.data, {}, use_tor, true) != DownloadResult::OK || !thumbnail_data->image->loadFromFile(thumbnail_path.data)) {
+                    if(download_to_file(url, thumbnail_path.data, {}, use_tor, true) != DownloadResult::OK || !load_image_from_file(*thumbnail_data->image, thumbnail_path.data)) {
                         thumbnail_data->loading_state = LoadingState::FINISHED_LOADING;
                         loading_image[free_index] = false;
                         return;
diff --git a/src/Body.cpp b/src/Body.cpp
index fd29d17..6b7826d 100644
--- a/src/Body.cpp
+++ b/src/Body.cpp
@@ -59,8 +59,8 @@ namespace QuickMedia {
     {
         progress_text.setFillColor(sf::Color::White);
         replies_text.setFillColor(sf::Color(129, 162, 190));
-        thumbnail_resize_target_size.x = 250;
-        thumbnail_resize_target_size.y = 141;
+        thumbnail_max_size.x = 250;
+        thumbnail_max_size.y = 141;
         item_background.setFillColor(sf::Color(55, 60, 68));
         sf::Vector2f loading_icon_size(loading_icon.getTexture()->getSize().x, loading_icon.getTexture()->getSize().y);
         loading_icon.setOrigin(loading_icon_size.x * 0.5f, loading_icon_size.y * 0.5f);
@@ -387,8 +387,8 @@ namespace QuickMedia {
             }
 
             update_dirty_state(item.get(), size);
-            float item_height = get_item_height(item.get());
             item->last_drawn_time = elapsed_time_sec;
+            float item_height = get_item_height(item.get());
 
             // This is needed here rather than above the loop, since update_dirty_text cant be called inside scissor because it corrupts the text for some reason
             glEnable(GL_SCISSOR_TEST);
@@ -509,9 +509,9 @@ namespace QuickMedia {
     sf::Vector2i Body::get_item_thumbnail_size(BodyItem *item) const {
         sf::Vector2i content_size;
         if(item->thumbnail_size.x > 0 && item->thumbnail_size.y > 0)
-            content_size = clamp_to_size(item->thumbnail_size, thumbnail_resize_target_size);
+            content_size = clamp_to_size(item->thumbnail_size, thumbnail_max_size);
         else
-            content_size = thumbnail_resize_target_size;
+            content_size = thumbnail_max_size;
         return content_size;
     }
 
diff --git a/src/ImageViewer.cpp b/src/ImageViewer.cpp
index fc7b758..c704485 100644
--- a/src/ImageViewer.cpp
+++ b/src/ImageViewer.cpp
@@ -1,6 +1,7 @@
 #include "../include/ImageViewer.hpp"
 #include "../include/Notification.hpp"
 #include "../include/Storage.hpp"
+#include "../include/SfmlFixes.hpp"
 #include "../plugins/Manga.hpp"
 #include <cmath>
 #include <SFML/Window/Event.hpp>
@@ -50,7 +51,7 @@ namespace QuickMedia {
             image_loader_thread.join();
         image_loader_thread = std::thread([this, image_data, path]() mutable {
             auto image = std::make_unique<sf::Image>();
-            if(image->loadFromFile(path.data)) {
+            if(load_image_from_file(*image, path.data)) {
                 image_data->image = std::move(image);
                 image_data->image_status = ImageStatus::LOADED;
             } else {
diff --git a/src/QuickMedia.cpp b/src/QuickMedia.cpp
index f944e7d..48ef01b 100644
--- a/src/QuickMedia.cpp
+++ b/src/QuickMedia.cpp
@@ -19,6 +19,7 @@
 #include "../include/base64_url.hpp"
 #include "../include/Entry.hpp"
 #include "../include/NetUtils.hpp"
+#include "../include/SfmlFixes.hpp"
 
 #include <assert.h>
 #include <cmath>
@@ -926,7 +927,7 @@ namespace QuickMedia {
         }
 
         for(Tab &tab : tabs) {
-            tab.body->thumbnail_resize_target_size = tab.page->get_max_thumbnail_size();
+            tab.body->thumbnail_max_size = tab.page->get_thumbnail_max_size();
         }
 
         const Json::Value *json_chapters = &Json::Value::nullSingleton();
@@ -1837,7 +1838,7 @@ namespace QuickMedia {
 
         if(get_file_type(image_path) == FileType::REGULAR && upscaled_ok) {
             sf::Image image;
-            if(image.loadFromFile(image_path.data)) {
+            if(load_image_from_file(image, image_path.data)) {
                 if(image_texture.loadFromImage(image)) {
                     image_texture.setSmooth(true);
                     image_texture.generateMipmap();
@@ -2346,7 +2347,8 @@ namespace QuickMedia {
             DownloadResult download_image_result = download_to_string(challenge_info.payload_url, payload_image_data, {}, is_tor_enabled());
             if(download_image_result == DownloadResult::OK) {
                 std::lock_guard<std::mutex> lock(captcha_image_mutex);
-                if(captcha_texture.loadFromMemory(payload_image_data.data(), payload_image_data.size())) {
+                sf::Image captcha_image;
+                if(load_image_from_memory(captcha_image, payload_image_data.data(), payload_image_data.size()) && captcha_texture.loadFromImage(captcha_image)) {
                     captcha_texture.setSmooth(true);
                     captcha_sprite.setTexture(captcha_texture, true);
                     challenge_description_text.setString(challenge_info.description);
@@ -2704,7 +2706,8 @@ namespace QuickMedia {
                     downloading_image = false;
                     image_data = load_image_future.get();
                     
-                    if(attached_image_texture->loadFromMemory(image_data.data(), image_data.size())) {
+                    sf::Image attached_image;
+                    if(load_image_from_memory(attached_image, image_data.data(), image_data.size()) && attached_image_texture->loadFromImage(attached_image)) {
                         attached_image_texture->setSmooth(true);
                         //attached_image_texture->generateMipmap();
                         attached_image_sprite.setTexture(*attached_image_texture, true);
@@ -2943,7 +2946,7 @@ namespace QuickMedia {
         messages_tab.type = ChatTabType::MESSAGES;
         messages_tab.body = std::make_unique<Body>(this, font.get(), bold_font.get(), cjk_font.get(), loading_icon);
         messages_tab.body->draw_thumbnails = true;
-        messages_tab.body->thumbnail_resize_target_size = CHAT_MESSAGE_THUMBNAIL_MAX_SIZE;
+        messages_tab.body->thumbnail_max_size = CHAT_MESSAGE_THUMBNAIL_MAX_SIZE;
         messages_tab.body->thumbnail_mask_shader = &circle_mask_shader;
         //messages_tab.body->line_separator_color = sf::Color::Transparent;
         messages_tab.text = sf::Text("Messages", *font, tab_text_size);
@@ -2967,8 +2970,28 @@ namespace QuickMedia {
         RoomData *current_room = nullptr;
         bool is_window_focused = window.hasFocus();
 
+        // Returns -1 if no rooms or no unread rooms
+        auto find_top_body_position_for_unread_room = [&tabs](BodyItem *item_to_swap, int start_index) {
+            for(int i = start_index; i < (int)tabs[ROOMS_TAB_INDEX].body->items.size(); ++i) {
+                const auto &body_item = tabs[ROOMS_TAB_INDEX].body->items[i];
+                if(static_cast<RoomData*>(body_item->userdata)->last_message_read || body_item.get() == item_to_swap)
+                    return i;
+            }
+            return -1;
+        };
+
+        // Returns -1 if no rooms or all rooms have unread mentions
+        auto find_top_body_position_for_mentioned_room = [&tabs](BodyItem *item_to_swap, int start_index) {
+            for(int i = start_index; i < (int)tabs[ROOMS_TAB_INDEX].body->items.size(); ++i) {
+                const auto &body_item = tabs[ROOMS_TAB_INDEX].body->items[i];
+                if(!static_cast<RoomData*>(body_item->userdata)->has_unread_mention || body_item.get() == item_to_swap)
+                    return i;
+            }
+            return -1;
+        };
+
         auto process_new_room_messages = 
-            [this, &selected_tab, &current_room, &is_window_focused, &tabs]
+            [this, &selected_tab, &current_room, &is_window_focused, &tabs, &find_top_body_position_for_unread_room, &find_top_body_position_for_mentioned_room]
             (RoomSyncMessages &room_sync_messages, bool is_first_sync) mutable
         {
             for(auto &[room, messages] : room_sync_messages) {
@@ -2982,7 +3005,6 @@ namespace QuickMedia {
                 }
             }
 
-            bool has_unread_messages = false;
             for(auto &[room, messages] : room_sync_messages) {
                 if(messages.empty())
                     continue;
@@ -3010,13 +3032,32 @@ namespace QuickMedia {
                 assert(room_body_item);
 
                 if(last_unread_message) {
-                    has_unread_messages = true;
                     std::string room_desc = "Unread: " + matrix->message_get_author_displayname(last_unread_message) + ": " + extract_first_line(last_unread_message->body, 150);
                     if(room->has_unread_mention)
                         room_desc += "\n** You were mentioned **"; // TODO: Better notification?
                     room_body_item->set_description(std::move(room_desc));
                     room_body_item->set_title_color(sf::Color(255, 100, 100));
                     room->last_message_read = false;
+
+                    // Swap order of rooms in body list to put rooms with mentions at the top and then unread messages and then all the other rooms
+                    // TODO: Optimize with hash map instead of linear search? or cache the index
+                    Body *rooms_body = tabs[ROOMS_TAB_INDEX].body.get();
+                    int room_body_index = rooms_body->get_index_by_body_item(room_body_item);
+                    if(room_body_index != -1) {
+                        std::shared_ptr<BodyItem> body_item = rooms_body->items[room_body_index];
+                        int body_swap_index = -1;
+                        if(room->has_unread_mention)
+                            body_swap_index = find_top_body_position_for_mentioned_room(body_item.get(), 0);
+                        else if(!room->last_message_read)
+                            body_swap_index = find_top_body_position_for_unread_room(body_item.get(), 0);
+                        if(body_swap_index != -1 && body_swap_index != room_body_index) {
+                            rooms_body->items.erase(rooms_body->items.begin() + room_body_index);
+                            if(body_swap_index < room_body_index)
+                                rooms_body->items.insert(rooms_body->items.begin() + body_swap_index, std::move(body_item));
+                            else
+                                rooms_body->items.insert(rooms_body->items.begin() + (body_swap_index - 1), std::move(body_item));
+                        }
+                    }
                 } else if(is_first_sync) {
                     Message *last_unread_message = nullptr;
                     for(auto it = messages.rbegin(), end = messages.rend(); it != end; ++it) {
@@ -3029,16 +3070,6 @@ namespace QuickMedia {
                         room_body_item->set_description(matrix->message_get_author_displayname(last_unread_message) + ": " + extract_first_line(last_unread_message->body, 150));
                 }
             }
-
-            if(has_unread_messages) {
-                std::sort(tabs[ROOMS_TAB_INDEX].body->items.begin(), tabs[ROOMS_TAB_INDEX].body->items.end(), [](std::shared_ptr<BodyItem> &item1, std::shared_ptr<BodyItem> &item2) {
-                    RoomData *item1_room = static_cast<RoomData*>(item1->userdata);
-                    RoomData *item2_room = static_cast<RoomData*>(item2->userdata);
-                    int item1_presence_sum = (int)!item1_room->last_message_read + (int)item1_room->has_unread_mention;
-                    int item2_presence_sum = (int)!item2_room->last_message_read + (int)item2_room->has_unread_mention;
-                    return item1_presence_sum > item2_presence_sum;
-                });
-            }
         };
 
         enum class ChatState {
diff --git a/src/SfmlFixes.cpp b/src/SfmlFixes.cpp
new file mode 100644
index 0000000..d6d4b17
--- /dev/null
+++ b/src/SfmlFixes.cpp
@@ -0,0 +1,15 @@
+#include "../include/SfmlFixes.hpp"
+#include <mutex>
+
+static std::mutex mutex;
+namespace QuickMedia {
+    bool load_image_from_file(sf::Image &image, const std::string &filepath) {
+        std::lock_guard<std::mutex> lock(mutex);
+        return image.loadFromFile(filepath);
+    }
+
+    bool load_image_from_memory(sf::Image &image, const void *data, size_t size) {
+        std::lock_guard<std::mutex> lock(mutex);
+        return image.loadFromMemory(data, size);
+    }
+}
\ No newline at end of file