LINUX.ORG.RU

Нафига так было делать?

 , , ,


0

4

Сабж касается сорцов inkscape, точнее одного мелкого бага. Глядя на количество открытых багов и активных разработчиков понял я, что если и дойдут у кого-то руки до фикса, то очень не скоро, т.к. даже подтвердить или опровергнуть не могут довольно долго. В итоге решил сам поковырять. Нашел причину в файле /inkscape/src/id-clash.cpp, а именно в функции

void rename_id(SPObject *elem, Glib::ustring const &new_name)
{
    if (new_name.empty()){
        g_message("Invalid Id, will not change.");
        return;
    }
    gchar *id = g_strdup(new_name.c_str()); //id is not empty here as new_name is check to be not empty
    g_strcanon (id, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.:", '_');
    Glib::ustring new_name2 = id; //will not fail as id can not be NULL, see length check on new_name
    g_free (id);
    if (!isalnum (new_name2[0])) {
        g_message("Invalid Id, will not change.");
        return;
    }

    SPDocument *current_doc = elem->document;
    refmap_type refmap;
    find_references(current_doc->getRoot(), refmap);

    std::string old_id(elem->getId());
    if (current_doc->getObjectById(id)) {
        // Choose a new ID.
        // To try to preserve any meaningfulness that the original ID
        // may have had, the new ID is the old ID followed by a hyphen
        // and one or more digits.
        new_name2 += '-';
        for (;;) {
            new_name2 += "0123456789"[std::rand() % 10];
            if (current_doc->getObjectById(new_name2) == NULL)
                break;
        }
    }

    // Change to the new ID
    elem->getRepr()->setAttribute("id", new_name2);
    // Make a note of this change, if we need to fix up refs to it
    id_changelist_type id_changes;
    if (refmap.find(old_id) != refmap.end()) {
        id_changes.push_back(id_changeitem_type(elem, old_id));
    }

    fix_up_refs(refmap, id_changes);
}
Вот как могло в голову прийти такое? Зачем они добавляют к концу имени свои циферки, когда используют эту функцию в том числе для ручной смены названия? Честно говоря, не вдупляю, зачем вообще это было делать? Как бы комментарий о том, что они не хотят терять старую информацию о ID мне ясен, но при использовании этой функции и так теряется всё, кроме нового значения и рандомных циферок на конце. Или им просто хочется гарантировать, чтобы новый id не совпадал с уже имеющимся (но всё равно это реализовано криво и логически неправильно)?

★★★★★

скорее всего последнее. каково поведении при дубляже ид? вылетает эксепшн, флоу уикс разрушается? пользвователь выпадает из потока? предложи свое решение, запили реализацию когда не-уникальное имя нельзя срхранить (прозреваю тормоза на больших файлах), скорей всего это один из тех случаев, называемых компромиссом. на крайняк введи понятие internal_name и display_title лол

anonymous ()

Судя по всему, они подразумевает ситуацию, что тот же id может иметь несколько элементов, а сменить надо только для одного.

PHPFan ()

Или им просто хочется гарантировать, чтобы новый id не совпадал с уже имеющимся

if (current_doc->getObjectById(id)) { ...

судя по коду, да. но для чего они освобождают id перед этим (g_free (id);) ?

ниже в условии используется new_name2, так что, скорее всего, в первом случае надо писать её же, но вообще, всё равно не понятно, так как вначале ей присваивается освобождающийся строчкой ниже указатель:

Glib::ustring new_name2 = id;
g_free (id);

может в цпп так принято? я хз. скорее всего это нормально.

и почему getObjectById(NULL) возвращает истину? если там NULL, конечно.

кароче, меняй id на new_name2 и отправляй патч )

anonymous ()

g_free (id);
...
if (current_doc->getObjectById(id)) {

Жуть. Вот поэтому я всегда оборачиваю работу с объектами, имеющими короткое время жизни, в отдельные блоки. Наподобие:

    Glib::ustring new_name2;
    {
        gchar *id = g_strdup(new_name.c_str());
        g_strcanon (id, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.:", '_');
        new_name2 = id;
        g_free (id);
    }
devzero ()
Ответ на: комментарий от anonymous

может в цпп так принято?

Это костыли. glibmm использует собственный велосипедный класс для строк (о, да!), а автор кода хочет позвать функцию g_strcanon() для обычной сишной строки. Поэтому ему приходится:

1. Выделить для id на куче память и скопировать туда строку.
2. Вызвать g_strcanon()
3. Проинициализировать new_name2, чтобы работать с этим объектом дальше (внутри конструктора повторно выделяется память на куче).
4. Освободить память id.

Обожаю говноклассы строк на крестах.

devzero ()
Ответ на: комментарий от devzero

Как я понимаю, после g_free(id) в id уже мусор? А дальше идет проверка непонятно чего?

peregrine ★★★★★ ()
Ответ на: комментарий от peregrine

Как я понимаю, после g_free(id) в id уже мусор?

После g_free(id), id указывает на свободный участок кучи. Если звёзды сложатся удачно, строка там не будет затёрта, и getObjectById(id) отработает как надо.

valgrind бы такое поймал сразу (да и cppcheck, скорее, всего тоже) но кто же будет проверять свои идеальный код под valgrind? :-)

devzero ()
Ответ на: комментарий от devzero

Ясно, спасибо, когда нет гарантий, что там будет то что надо я считаю что в переменной мусор. Вечерком или завтра посмотрю как это можно переписать по человечески (заодно на getObjectById погляжу). Мне сама суть этой проверки не нравится ко всему прочему. Баг не из-за мусора происходит (если только там не NULL, а getObjectById на NULL не возвращает истину), а из-за сути действий с new_name2.

peregrine ★★★★★ ()
Ответ на: комментарий от anonymous

всякие copy/free вообще моветон и крест головного мозга, дети, позвольте ресурсами управлять машине

anonymous ()
Ответ на: комментарий от anonymous

может в цпп так принято? я хз. скорее всего это нормально.

Нет, это просто обычный C-говнокод.

asaw ★★★★★ ()
Ответ на: комментарий от asaw

Нет, это просто обычный говнокод на крестах.

Пофиксил.

devzero ()

Обсуждение кода на лоре всегда напоминает какое-то сборище манерных девочек. Обычный баг в обычном коде, запости репорт и его пофиксят, если еще не

anonymous ()
Ответ на: комментарий от devzero

Нет, это просто обычный говнокод на крестах.

Хорошо, это обычный C-говнокод в C++.

asaw ★★★★★ ()
Ответ на: комментарий от anonymous

запости репорт и его пофиксят

К тому времени, когда у меня борода станет совсем седой. Репорт уже неделю висит, вместе с другими 4k багов и хотелок в проекте. Его даже не подтвердят никак.

peregrine ★★★★★ ()

Теперь понятно почему Inkscape такой унылый. Врагу не посоветуешь такой код поддерживать.

У вас баг не в цвете, а в самом swatch. Это же просто градиент. То есть это даже не svg баг, а inkscape-специфичный баг.

По логике оно пытается создать уникальный id без проверки на существование такового. То есть они должны проверять, что id, введенного пользователем, нет и оставлять его. Если есть - генерировать циферки.

RazrFalcon ★★★★★ ()

Подтверждаю этот баг. Могу рассказать ещё один, с подобным же уровнем маразма, на который можно напороться при работе с палитрой.

  1. Создаём объект, даём ему заливку, регулируем значение альфа этой заливки и превращаем её в swatch.
  2. Создаём второй объект, даём ему заливку того же цвета, назначаем уже другое значение по альфе и тоже превращаем эту заливку в swatch.
  3. Теперь при копировании одного из объектов через Ctrl+C Ctrl+V его swatch заливки будет слетать на чужой swatch, так как в коде зачем-то при копировании swatch ищется заново, но без учёта значения альфы.

Конкретное место не скажу, но находится довольно легко тоже.

zezic ★★★ ()

Не прошло и недели и я нашел время настроить сборку inkscape из исходников и замутил патч по этому багу (по ссылке в первом посте). На вид всё работает прекрасно. По изначальной логике скорее всего тоже. Однако, буду благодарен, если баг кто-нибудь подтвердит на ланчпаде, иначе висеть моему патчу в багтрекере не один месяц.

peregrine ★★★★★ ()
Ответ на: комментарий от peregrine

Довольно быстро патч приняли. И тоже спасибо, кстати :)

anonymous ()
Вы не можете добавлять комментарии в эту тему. Тема перемещена в архив.